* Re: [PATCH v2 20/25] sequencer: left-trim lines read from the script
From: Junio C Hamano @ 2016-09-12 15:42 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1609121019290.129229@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> I do not offhand see why we want to be lenient here,
>> especially only to the left.
>
> Postel's Law.
How would that compare/relate to yagni, though?
^ permalink raw reply
* Re: [PATCH v3 0/2] patch-id for merges
From: Jeff King @ 2016-09-12 15:59 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Michael Haggerty, Kevin Willford, Xiaolong Ye,
Johannes Schindelin, Josh Triplett
In-Reply-To: <xmqqfup8aiud.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 09, 2016 at 02:01:14PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > And here is v3. Besides commit-message fixups, it drops patch 2, and
> > instead the third patch teaches commit_patch_id() to distinguish between
> > errors and "no patch id".
> >
> > Frankly, I still like v2 better, but I do not feel like arguing with
> > Johannes about it anymore.
>
> FWIW, I too like the simplicity of v2, as all the error-to-die
> conversion is for cases in which there is no sane recovery path.
>
> I'll have to take a bit deeper look at [v3 2/2] that had to become
> more involved to decide if the additional flexibility is really
> worth it.
One other option I didn't really look at: commit_patch_id() could
consider feeding it a merge as an error, and it would be come the
caller's responsibility to avoid doing so. That should already be the
case for "format-patch --base".
We'd probably have to change add_commit_patch_id() and
has_commit_patch_id() to return NULL early when fed a merge, but that is
not too bad.
The reason I didn't pursue this is that I didn't want the definition of
"what constitutes something with no patch-id" to cross too many
abstraction layers. But it's not like we expect a multitude of
conditions; it will probably remain just "we don't handle merges" for
the foreseeable future.
That looks like the patch below (as a replacement for patch 2), which is
even less invasive. It also performs a little better on my example case,
because we avoid adding merges to the hashmap entirely.
diff --git a/patch-ids.c b/patch-ids.c
index 77e4663..5d2d96a 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -7,10 +7,12 @@
int commit_patch_id(struct commit *commit, struct diff_options *options,
unsigned char *sha1, int diff_header_only)
{
- if (commit->parents)
+ if (commit->parents) {
+ if (commit->parents->next)
+ return -1;
diff_tree_sha1(commit->parents->item->object.oid.hash,
commit->object.oid.hash, "", options);
- else
+ } else
diff_root_tree_sha1(commit->object.oid.hash, "", options);
diffcore_std(options);
return diff_flush_patch_id(options, sha1, diff_header_only);
@@ -72,11 +74,20 @@ static int init_patch_id_entry(struct patch_id *patch,
return 0;
}
+static int patch_id_defined(struct commit *commit)
+{
+ /* must be 0 or 1 parents */
+ return !commit->parents || !commit->parents->next;
+}
+
struct patch_id *has_commit_patch_id(struct commit *commit,
struct patch_ids *ids)
{
struct patch_id patch;
+ if (!patch_id_defined(commit))
+ return NULL;
+
memset(&patch, 0, sizeof(patch));
if (init_patch_id_entry(&patch, commit, ids))
return NULL;
@@ -89,6 +100,9 @@ struct patch_id *add_commit_patch_id(struct commit *commit,
{
struct patch_id *key = xcalloc(1, sizeof(*key));
+ if (!patch_id_defined(commit))
+ return NULL;
+
if (init_patch_id_entry(key, commit, ids)) {
free(key);
return NULL;
I'd probably do a preparatory patch to drop the return value from
add_commit_patch_id(). No callers actually look at it.
-Peff
^ permalink raw reply related
* Re: [RFC/PATCH] ls-files: adding support for submodules
From: Junio C Hamano @ 2016-09-12 16:01 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, Brandon Williams, git@vger.kernel.org
In-Reply-To: <20160912005229.6njhgfq7h6cb34s4@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> I do not use submodules myself, but I could imagine that you may have
>> scripts outside of git that do not care about the submodule divisions at
>> all, and would be happy with the flat block. ...
>> git-grep does not (I don't use "ack", but perhaps "git ls-files
>> --recurse-submodules -z | xargs --null ack ..." is something people
>> would want to do).
>
> None of that negates your point, btw, which is that this does not seem
> like a great building block for "git grep --recurse-submodules". Just
> that it seems plausible to me that people could find recursive
> "ls-files" useful on its own.
I do think it is a good argument why ls-files (with or without -z)
that recurses into submodules would help "git grep" that does not
look at the object store and does the search in the working tree
files that are tracked. In normal Git world view, if you want a
list of files that are tracked, you are the one who is supposed to
go to the repository and look at the index there, but if you can
somehow magically get that list out-of-band, the "in the working
tree files" mode of "git grep" can work just like the "xargs -z ack"
pipeline in your example.
So I tend to agree that the enhancement in question is a useful
thing on its own, at least the part that gives a list of paths in
the index.
I do not know if all other operationg modes are useful, though. For
eaxmple, the mode that lists untracked paths might be useful to do a
recursive "git clean". On the other hand, "ls-files -s" with the
patch may produce the "correct" result (i.e. the object name given
for each path would be the same one that are found in the index of
the submodule the path was taken from), but the correctness may not
necessarily translate to usefulness, exactly because you need to
know which submodule's repository has the named object that is not
given by the flattened list.
Having to say that "this command produces correct result, but not
all correct things are necessarily useful" makes me wonder if that
is the direction in which we would want to go. For example, what
would be our answer to an end-user question: I now know that the
recursive ls-files can give me all the untracked files in the
top-level and submodules. How can I feed that to "git add" to
update their respective index files with them?
I am not convinced that we would always want to make "git add
lib/Makefile" to automagically run "git -C lib/ add Makefile" when
lib/ is a submodule (for that matter, even if we wanted to, I do not
know if that is something we can efficiently do with pathspecs that
have wildcards).
So...
^ permalink raw reply
* Re: [PATCH v2 07/14] i18n: merge-recursive: mark error messages for translation
From: Junio C Hamano @ 2016-09-12 16:04 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA
In-Reply-To: <1473679802-31381-7-git-send-email-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> Lowercase first word of such error messages following the usual style.
"Change X to lowercase" is fine, but "Lowercase" is not a verb.
Reword it to "Downcase the first word...", perhaps (not limited to
this step).
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2016, #03; Fri, 9)
From: Jeff King @ 2016-09-12 16:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqoa3w8zco.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 09, 2016 at 03:47:35PM -0700, Junio C Hamano wrote:
> * jk/delta-base-cache (2016-08-23) 7 commits
> (merged to 'next' on 2016-08-25 at f1c141a)
> + t/perf: add basic perf tests for delta base cache
> + delta_base_cache: use hashmap.h
> + delta_base_cache: drop special treatment of blobs
> + delta_base_cache: use list.h for LRU
> + release_delta_base_cache: reuse existing detach function
> + clear_delta_base_cache_entry: use a more descriptive name
> + cache_or_unpack_entry: drop keep_cache parameter
>
> The delta-base-cache mechanism has been a key to the performance in
> a repository with a tightly packed packfile, but it did not scale
> well even with a larger value of core.deltaBaseCacheLimit.
I happened to notice today that this topic needs a minor tweak:
-- >8 --
Subject: [PATCH] add_delta_base_cache: use list_for_each_safe
We may remove elements from the list while we are iterating,
which requires using a second temporary pointer. Otherwise
stepping to the next element of the list might involve
looking at freed memory (which generally works in practice,
as we _just_ freed it, but of course is wrong to rely on;
valgrind notices it).
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_file.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index a57b71d..132c861 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2187,11 +2187,11 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,
void *base, unsigned long base_size, enum object_type type)
{
struct delta_base_cache_entry *ent = xmalloc(sizeof(*ent));
- struct list_head *lru;
+ struct list_head *lru, *tmp;
delta_base_cached += base_size;
- list_for_each(lru, &delta_base_cache_lru) {
+ list_for_each_safe(lru, tmp, &delta_base_cache_lru) {
struct delta_base_cache_entry *f =
list_entry(lru, struct delta_base_cache_entry, lru);
if (delta_base_cached <= delta_base_cache_limit)
--
2.10.0.230.g6f8d04b
^ permalink raw reply related
* Re: [PATCH v2 07/14] i18n: merge-recursive: mark error messages for translation
From: Junio C Hamano @ 2016-09-12 17:01 UTC (permalink / raw)
To: Jean-Noël Avila
Cc: Vasco Almeida, git, Jiang Xin,
Ævar Arnfjörð Bjarmason, Jean-Noël AVILA
In-Reply-To: <882dcaf3-fd9b-0d83-2756-f3315dd6bb4e@gmail.com>
Jean-Noël Avila <avila.jn@gmail.com> writes:
> Le 12/09/2016 à 13:29, Vasco Almeida a écrit :
>> - warning("Cannot handle more than %d bases. "
>> - "Ignoring %s.",
>> + warning(_("cannot handle more than %d bases. "
>> + "Ignoring %s."),
>> (int)ARRAY_SIZE(bases)-1, argv[i]);
>> }
>
> This one is also missing the plural form.
That is technically correct, but I do not think it matters, as we
won't be making bases[] array smaller than it currently is, so the
"more than %d bases" part of the message will always be in the
plural form and never be singular.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 12/14] i18n: show-branch: mark error messages for translation
From: Junio C Hamano @ 2016-09-12 17:02 UTC (permalink / raw)
To: Jean-Noël Avila
Cc: Vasco Almeida, git, Jiang Xin,
Ævar Arnfjörð Bjarmason
In-Reply-To: <ba3430ff-9643-eddb-31e7-c1e9a399d806@free.fr>
Jean-Noël Avila <jn.avila@free.fr> writes:
> Le 12/09/2016 à 13:30, Vasco Almeida a écrit :
>> if (MAX_REVS < reflog)
>> - die("Only %d entries can be shown at one time.",
>> + die("only %d entries can be shown at one time.",
>> MAX_REVS);
>
> Wouldn't you i18n this one too, with plural form?
I would think this one is OK in practice for the same reason as 7/14
^ permalink raw reply
* Re: [PATCH v3 0/2] patch-id for merges
From: Junio C Hamano @ 2016-09-12 17:18 UTC (permalink / raw)
To: Jeff King
Cc: git, Michael Haggerty, Kevin Willford, Xiaolong Ye,
Johannes Schindelin, Josh Triplett
In-Reply-To: <20160912155930.2acw4265nfjq3uyj@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> That looks like the patch below (as a replacement for patch 2), which is
> even less invasive. It also performs a little better on my example case,
> because we avoid adding merges to the hashmap entirely.
After reading it, I did not find [v3 2/2] is too bad either, but
this is even better ;-)
> diff --git a/patch-ids.c b/patch-ids.c
> index 77e4663..5d2d96a 100644
> --- a/patch-ids.c
> +++ b/patch-ids.c
> @@ -7,10 +7,12 @@
> int commit_patch_id(struct commit *commit, struct diff_options *options,
> unsigned char *sha1, int diff_header_only)
> {
> - if (commit->parents)
> + if (commit->parents) {
> + if (commit->parents->next)
> + return -1;
> diff_tree_sha1(commit->parents->item->object.oid.hash,
> commit->object.oid.hash, "", options);
> - else
> + } else
> diff_root_tree_sha1(commit->object.oid.hash, "", options);
This looks familiar ;-)
> @@ -72,11 +74,20 @@ static int init_patch_id_entry(struct patch_id *patch,
> return 0;
> }
>
> +static int patch_id_defined(struct commit *commit)
> +{
> + /* must be 0 or 1 parents */
> + return !commit->parents || !commit->parents->next;
> +}
If we make the first hunk begin like so:
> + if (commit->parents) {
> + if (!patch_id_defined(commit))
> + return -1;
I wonder if the compiler gives us the same code.
> struct patch_id *has_commit_patch_id(struct commit *commit,
> struct patch_ids *ids)
> {
> struct patch_id patch;
>
> + if (!patch_id_defined(commit))
> + return NULL;
> +
> memset(&patch, 0, sizeof(patch));
> if (init_patch_id_entry(&patch, commit, ids))
> return NULL;
> @@ -89,6 +100,9 @@ struct patch_id *add_commit_patch_id(struct commit *commit,
> {
> struct patch_id *key = xcalloc(1, sizeof(*key));
>
> + if (!patch_id_defined(commit))
> + return NULL;
> +
> if (init_patch_id_entry(key, commit, ids)) {
> free(key);
> return NULL;
Yup, these two hunks look a lot nicer.
> I'd probably do a preparatory patch to drop the return value from
> add_commit_patch_id(). No callers actually look at it.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 12/14] i18n: show-branch: mark error messages for translation
From: Jean-Noël AVILA @ 2016-09-12 17:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: Vasco Almeida, git, Jiang Xin,
Ævar Arnfjörð Bjarmason
In-Reply-To: <xmqqa8fd3vb5.fsf@gitster.mtv.corp.google.com>
On lundi 12 septembre 2016 10:02:54 CEST Junio C Hamano wrote:
> Jean-Noël Avila <jn.avila@free.fr> writes:
> > Le 12/09/2016 à 13:30, Vasco Almeida a écrit :
> >> if (MAX_REVS < reflog)
> >>
> >> - die("Only %d entries can be shown at one time.",
> >> + die("only %d entries can be shown at one time.",
> >>
> >> MAX_REVS);
> >
> > Wouldn't you i18n this one too, with plural form?
>
> I would think this one is OK in practice for the same reason as 7/14
I would think the contrary for the same reason as 14/14
We don't know the actual value of MAX_REVS at the time we translate, and
plural forms can be quite different depending on its value. See for instance
polish:
https://www.gnu.org/software/gettext/manual/html_node/Plural-forms.html
Of course, too bad, we only use one of the forms but I don't see any
alternative.
Thanks,
JN
^ permalink raw reply
* Re: [PATCH 1/2 v5] pack-objects: respect --local/--honor-pack-keep/--incremental when bitmap is in use
From: Junio C Hamano @ 2016-09-12 17:33 UTC (permalink / raw)
To: Kirill Smelkov
Cc: Jeff King, Vicent Marti, Jérome Perrin, Isabelle Vallet,
Kazuhiko Shiozaki, Julien Muchembled, git
In-Reply-To: <20160910145738.x6ihp2gqzpk7dbi3@teco.navytux.spb.ru>
Kirill Smelkov <kirr@nexedi.com> writes:
> On Thu, Aug 18, 2016 at 01:52:22PM -0400, Jeff King wrote:
> >
> > Good to know there is no regression. It is curious that there is a
> > slight _improvement_ across the board. Do we have an explanation for
> > that? It seems odd that noise would be so consistent.
>
> Yes, I too thought it and it turned out to be t/perf/run does not copy
> config.mak.autogen & friends to build/ and I'm using autoconf with
> CFLAGS="-march=native -O3 ..."
>
> Junio, I could not resist to the following:
> ...
> With corrected t/perf/run the timings are more realistic - e.g. 3
> consecutive runs of `./run 56dfeb62 . ./p5310-pack-bitmaps.sh`:
Wow, that's what I call an exchange with quality during a review ;-)
Thanks for the curiosity and digging it to the root cause of the
anomaly. Some GNUism/bashism in the way copying is spelled in the
patch bothers me, but that is easily fixable.
Thanks.
^ permalink raw reply
* Re: [RFC/PATCH] ls-files: adding support for submodules
From: Brandon Williams @ 2016-09-12 17:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Stefan Beller, git@vger.kernel.org
In-Reply-To: <CAKoko1oSEac_Nr1SkRB=dM_r3Jnew1Et2ZKj716iU3JLyHe2GQ@mail.gmail.com>
Thanks for all the comments. What it sounds like is that using ls-files as
a means to power a recursive git-grep may not be like the best approach (I
assumed that would be the case but thought it a decent place to start).
I agree that not all operating modes would be useful for a recursive
ls-files, which is why I initially don't have support for them. I guess
the question would be which modes would be worth supporting in a recursive
case?
^ permalink raw reply
* Re: [PATCH v3 0/2] patch-id for merges
From: Jeff King @ 2016-09-12 17:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Michael Haggerty, Kevin Willford, Xiaolong Ye,
Johannes Schindelin, Josh Triplett
In-Reply-To: <xmqq37l53ul2.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 12, 2016 at 10:18:33AM -0700, Junio C Hamano wrote:
> > +static int patch_id_defined(struct commit *commit)
> > +{
> > + /* must be 0 or 1 parents */
> > + return !commit->parents || !commit->parents->next;
> > +}
>
> If we make the first hunk begin like so:
>
> > + if (commit->parents) {
> > + if (!patch_id_defined(commit))
> > + return -1;
>
> I wonder if the compiler gives us the same code.
Good idea. I actually put the "patch_id_defined" check outside the "if"
block you've quoted (otherwise we're making assumptions about the
contents of patch_id_defined).
I didn't check that the compiler generates the same code, but I'm
willing to blindly put faith in it. Either it will inline
patch_id_defined and optimize out the double-conditional, or it probably
doesn't matter in practice. Either way, the compiler is probably smarter
than me, and we should shoot for readability and not repeating
ourselves.
> > I'd probably do a preparatory patch to drop the return value from
> > add_commit_patch_id(). No callers actually look at it.
I decided against this. Technically add_commit_patch_id() can return an
error via the header-only diff_flush_patch_id(), and we'd be shutting
that down. Of course no callers actually _care_ about that right now, so
it doesn't matter at this point. But I'd prefer to punt it down the line
for when somebody does (and the solution may be to distinguish between
those two return codes, or it may be for the caller to have access to
patch_id_defined(); we won't know until we see the code).
So here's the replacement for 2/2:
-- >8 --
Subject: [PATCH] patch-ids: refuse to compute patch-id for merge commit
The patch-id code which powers "log --cherry-pick" doesn't
look at whether each commit is a merge or not. It just feeds
the commit's first parent to the diff, and ignores any
additional parents.
In theory, this might be useful if you wanted to find
equivalence between, say, a merge commit and a squash-merge
that does the same thing. But it also promotes a false
equivalence between distinct merges. For example, every
"merge -s ours" would look identical to an empty commit
(which is true in a sense, but presumably there was a value
in merging in the discarded history). Since patch-ids are
meant for throwing away duplicates, we should err on the
side of _not_ matching such merges.
Moreover, we may spend a lot of extra time computing these
merge diffs. In the case that inspired this patch, a "git
format-patch --cherry-pick" dropped from over 3 minutes to
less than 3 seconds.
This seems pretty drastic, but is easily explained. The
command was invoked by a "git rebase" of an older topic
branch; there had been tens of thousands of commits on the
upstream branch in the meantime. In addition, this project
used a topic-branch workflow with occasional "back-merges"
from "master" to each topic (to resolve conflicts on the
topics rather than in the merge commits). So there were not
only extra merges, but the diffs for these back-merges were
generally quite large (because they represented _everything_
that had been merged to master since the topic branched).
This patch treats a merge fed to commit_patch_id() or
add_commit_patch_id() as an error, and a lookup for such a
merge via has_commit_patch_id() will always return NULL.
An earlier version of the patch tried to distinguish between
"error" and "patch id for merges not defined", but that
becomes unnecessarily complicated. The only callers are:
1. revision traversals which want to do --cherry-pick;
they call add_commit_patch_id(), but do not care if it
fails. They only want to add what we can, look it up
later with has_commit_patch_id(), and err on the side
of not-matching.
2. format-patch --base, which calls commit_patch_id().
This _does_ notice errors, but should never feed a
merge in the first place (and if it were to do so
accidentally, then this patch is a strict improvement;
we notice the bug rather than generating a bogus
patch-id).
So in both cases, this does the right thing.
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Jeff King <peff@peff.net>
---
patch-ids.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/patch-ids.c b/patch-ids.c
index 77e4663..ce285c2 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -4,9 +4,18 @@
#include "sha1-lookup.h"
#include "patch-ids.h"
+static int patch_id_defined(struct commit *commit)
+{
+ /* must be 0 or 1 parents */
+ return !commit->parents || !commit->parents->next;
+}
+
int commit_patch_id(struct commit *commit, struct diff_options *options,
unsigned char *sha1, int diff_header_only)
{
+ if (!patch_id_defined(commit))
+ return -1;
+
if (commit->parents)
diff_tree_sha1(commit->parents->item->object.oid.hash,
commit->object.oid.hash, "", options);
@@ -77,6 +86,9 @@ struct patch_id *has_commit_patch_id(struct commit *commit,
{
struct patch_id patch;
+ if (!patch_id_defined(commit))
+ return NULL;
+
memset(&patch, 0, sizeof(patch));
if (init_patch_id_entry(&patch, commit, ids))
return NULL;
@@ -89,6 +101,9 @@ struct patch_id *add_commit_patch_id(struct commit *commit,
{
struct patch_id *key = xcalloc(1, sizeof(*key));
+ if (!patch_id_defined(commit))
+ return NULL;
+
if (init_patch_id_entry(key, commit, ids)) {
free(key);
return NULL;
--
2.10.0.230.g6f8d04b
^ permalink raw reply related
* Re: Git Miniconference at Plumbers
From: Jon Loeliger @ 2016-09-12 18:09 UTC (permalink / raw)
To: David Bainbridge; +Cc: Jeff King, git@vger.kernel.org
In-Reply-To: <DB5PR07MB1448B5EDFE2E2D84C42A8AFCE2FF0@DB5PR07MB1448.eurprd07.prod.outlook.com>
So, like, David Bainbridge said:
> Hi,
>
> The subject matter of the conference looks really interesting but I am
> unlikely to be able to attend, unfortunately.
>
> The subjects being covered like the current State of Git and the
> Future of Git, for example, deserve much wider exposure, and I would
> certainly appreciate hearing the thoughts of Junio and others.
Indeed.
> Does anyone know whether the sessions will be recorded in any way?
I am uncertain about outright recording (digital video/audio),
but there will be at least summarizing notes taken and posted.
Anyone wishing to record the talks/discussions is likely welcome
to do so.
HTH,
jdl
^ permalink raw reply
* RE: Git Miniconference at Plumbers
From: David Bainbridge @ 2016-09-12 17:53 UTC (permalink / raw)
To: Jon Loeliger, Jeff King; +Cc: git@vger.kernel.org
In-Reply-To: <E1bjRLd-0005k0-Vb@mylo.jdl.com>
Hi,
The subject matter of the conference looks really interesting but I am unlikely to be able to attend, unfortunately.
The subjects being covered like the current State of Git and the Future of Git, for example, deserve much wider exposure, and I would certainly appreciate hearing the thoughts of Junio and others.
Does anyone know whether the sessions will be recorded in any way?
Thanks
David
DAVID BAINBRIDGE
Product Manager SW Development
Ericsson
8500 Decarie
Montreal, H4P 2N2, Canada
david.bainbridge@ericsson.com
www.ericsson.com
-----Original Message-----
From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of Jon Loeliger
Sent: Monday, September 12, 2016 09:33
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: Git Miniconference at Plumbers
So, like, Jeff King said:
> On Tue, Sep 06, 2016 at 12:42:04PM -0500, Jon Loeliger wrote:
>
> > I have recently been enlisted by folks at the Linux Foundation to
> > help run a Miniconference on Git at the Plumbers Conference [*] this
> > fall.
>
> I see the conference runs for 4 days; I assume the Git portion will
> just be one day.
Yes, and yes. Likely even "a half day".
> Do you know yet which day?
No. Sorry.
> -Peff
jdl
^ permalink raw reply
* RE: [PATCH v2] checkout: eliminate unnecessary merge for trivial checkout
From: Ben Peart @ 2016-09-12 18:12 UTC (permalink / raw)
To: 'Junio C Hamano'; +Cc: git, pclouds, 'Ben Peart'
In-Reply-To: <xmqq1t0sagcm.fsf@gitster.mtv.corp.google.com>
> -----Original Message-----
> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Friday, September 9, 2016 5:55 PM
> To: Ben Peart <peartben@gmail.com>
> Cc: git@vger.kernel.org; pclouds@gmail.com; Ben Peart
> <benpeart@microsoft.com>
> Subject: Re: [PATCH v2] checkout: eliminate unnecessary merge for trivial
> checkout
>
> Ben Peart <peartben@gmail.com> writes:
>
> > @@ -802,6 +806,87 @@ static void orphaned_commit_warning(struct
> commit *old, struct commit *new)
> > free(refs.objects);
> > }
> >
> > +static int needs_working_tree_merge(const struct checkout_opts *opts,
> > + const struct branch_info *old,
> > + const struct branch_info *new)
> > +{
> > + /*
> > + * We must do the merge if we are actually moving to a new
> > + * commit tree.
> > + */
> > + if (!old->commit || !new->commit ||
> > + oidcmp(&old->commit->tree->object.oid, &new->commit-
> >tree->object.oid))
> > + return 1;
>
> A huge helper function helps it somewhat, compared with the earlier
> unreadable mess ;-).
>
> Are we certain that at this point the commit objects are both parsed and
> their tree->object.oid are both valid?
>
> > + /*
> > + * Honor the explicit request for a three-way merge or to throw away
> > + * local changes
> > + */
> > + if (opts->merge || opts->force)
> > + return 1;
>
> Hmph, "git checkout -m HEAD" wouldn't have to do anything wrt the index
> status, no?
>
> For that matter, neither "git checkout -f HEAD". Unless we rely on
> unpack_trees() to write over the working tree files.
>
> ... me goes and looks, and finds that merge_working_tree()
> indeed does have a logic to do quite different thing when
> "--force" is given.
>
> This makes me wonder if the "merge_working_tree() is expensive, so
> selectively skip calling it" approach is working at a wrong level.
> Wouldn't the merge_working_tree() function itself a better place to do
this
> kind of "we may not have to do the full two-way merge"
> optimization? It already looks at opts and does things differently (e.g.
when
> running with "--force", it does not even call unpack).
> If you can optimize even more by looking at other fields in opts to avoid
> unpack, that would fit better with the structure of the code that we
already
> have.
>
I completely agree that optimizing within merge_working_tree would provide
more opportunities for optimization. I can certainly move the test into
that
function as a first step. I've looked into it a little but came to the
conclusion
that it will be non-trivial to determine how to ensure the minimal work is
done for any arbitrary set of options passed in without breaking something.
While I'd love to see that work done, I just don't have the time to pursue
further
optimizations that may be available at this point in time. There are other
things
(like speeding up status on large repos) I need to work on first.
> > + /*
> > + * Checking out the requested commit may require updating the
> working
> > + * directory and index, let the merge handle it.
> > + */
> > + if (opts->force_detach)
> > + return 1;
>
> This does not make much sense to me. After "git branch -f foo HEAD",
there
> is no difference in what is done to the index and the working directory
> between "git checkout --detach HEAD" and "git checkout foo", is there?
>
I'm attempting to optimize for a single, common path where checkout is
just creating a new branch (ie "git checkout -b foo") to minimize the
possibility that I broke some other path I didn't fully understand.
It is quite possible that there are cases where the index and/or working
directory do not need to be updated or where a merge won't actually
change anything that this test is not optimized for. Perhaps I should
emphasize the "*may* require updating the working directory" in my
comment. Because it *could* happen, I let the code fall back to the
old behavior.
> > + /*
> > + * opts->writeout_stage cannot be used with switching branches so is
> > + * not tested here
> > + */
> > +
> > + /*
> > + * Honor the explicit ignore requests
> > + */
> > + if (!opts->overwrite_ignore || opts->ignore_skipworktree
> > + || opts->ignore_other_worktrees)
> > + return 1;
>
> Style. I think you earlier had
>
> if (a || b ||
> c)
>
> and here you are doing
>
> if (a || b
> || c)
>
> Please pick one and stick to it (I'd pick the former).
Done
>
> > + /*
> > + * If we're not creating a new branch, by definition we're changing
> > + * the existing one so need to do the merge
> > + */
> > + if (!opts->new_branch)
> > + return 1;
>
> Sorry, but I fail to follow that line of thought. Starting from a state
where
> your HEAD points at commit A,
>
> - switching to a detached HEAD pointing at a commit A,
> - switching to an existing branch that already points at the same
> commit A, and
> - force updating an existing branch that was pointing at something
> else to point at the same commit A,
>
> would have the same effect as creating a new branch at commit A and
> switching to it, no? The same comment applies to the remainder of this
> function.
>
> More importantly, merge_working_tree() checks things other than what this
> function is checking. For example, it prevents you from branch-switching
> (whether it is to switch to an existing branch that has the same commit as
the
> current HEAD, to switch to detached HEAD state at the same commit as the
> current HEAD, or to switch to a new branch that points at the same commit
> as the current HEAD) if your index is unmerged (i.e. you are in the middle
of
> a mergy operation).
>
> So my gut feeling is that this:
>
> > + /*
> > + * Optimize the performance of "git checkout foo" by skipping the
call
> > + * to merge_working_tree where possible.
> > + */
> > + if (needs_working_tree_merge(opts, &old, new)) {
> > + ret = merge_working_tree(opts, &old, new,
> &writeout_error);
>
> works at the wrong level. The comment up to 'Optimize the performance of
> "git checkout foo"' may correctly state what we want to achieve, but I
think
> we should do so not with "by skipping the call to", but with "by
optimizing
> merge_working_tree()".
>
I agree that optimizing merge_working_tree could result in even greater
savings and could definitely optimize for more paths/options than this
patch. While I'd love to see that done, I'm also happy to get a 10x
improvement in the common case of creating a new branch.
I'll reroll the patch moving the current optimization into
merge_working_tree and fixing up the style issues you pointed out.
> Thanks.
>
^ permalink raw reply
* Re: [PATCH v2 2/5] pull: make code more similar to the shell script again
From: Junio C Hamano @ 2016-09-12 18:56 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <c448851c24599d73143bce90984b9efc43d4a7aa.1473580914.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> When converting the pull command to a builtin, the
> require_clean_work_tree() function was renamed and the pull-specific
> parts hard-coded.
>
> This makes it impossible to reuse the code, so let's modify the code to
> make it more similar to the original shell script again.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/pull.c | 29 +++++++++++++++++++----------
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index d4bd635..a3ed054 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -365,10 +365,11 @@ static int has_uncommitted_changes(void)
> * If the work tree has unstaged or uncommitted changes, dies with the
> * appropriate message.
> */
> -static void die_on_unclean_work_tree(void)
> +static int require_clean_work_tree(const char *action, const char *hint,
> + int gently)
> {
> struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
> - int do_die = 0;
> + int err = 0;
>
> hold_locked_index(lock_file, 0);
> refresh_cache(REFRESH_QUIET);
> @@ -376,20 +377,27 @@ static void die_on_unclean_work_tree(void)
> rollback_lock_file(lock_file);
>
> if (has_unstaged_changes()) {
> - error(_("Cannot pull with rebase: You have unstaged changes."));
> - do_die = 1;
> + error(_("Cannot %s: You have unstaged changes."), _(action));
> + err = 1;
> }
> ...
> + error(_("Cannot %s: Your index contains uncommitted changes."),
> + _(action));
> + err = 1;
These are much better than the one in v1.
Depending on the target language, the translators may have to phrase
these not like "Cannot <verb>:" but "Cannot perform <noun>:" where
the "<noun>" is for "the act of doing <verb>", if the "cannot" part
in their language needs to change shape depending on the verb.
Hence, I think the translators need a /* TRANSLATORS: ... */ comment
that tells them what is interpolated here are their translations for
phrases like "pull with rebase". You do not have to be exhausitive
in the comment; a representative example would help the translators
see the message in context.
Other than that (and the need to further clean-up error() and die()
to begin with lower-case to match the modern practice in a separate
follow-up series), this looks ready to be queued.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 3/5] Make the require_clean_work_tree() function truly reusable
From: Junio C Hamano @ 2016-09-12 18:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <0e1ec34e45fea5bae60e65f316072cb2c89b024a.1473580914.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> It is remarkable that libgit.a did not sport this function yet... Let's
> move it into a more prominent (and into an actually reusable) spot:
> wt-status.[ch].
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
I do not think "truly" is needed at all.
It was not reusable. It is somewhat reusable after this patch. It
may or may not be "truly" reusable depending on the need of future
patches, which we do not know yet.
I agree wt-status.[ch] is a good home for this feature.
Thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2016, #03; Fri, 9)
From: Junio C Hamano @ 2016-09-12 19:10 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20160912164616.vg33kldazuthff3d@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I happened to notice today that this topic needs a minor tweak:
>
> -- >8 --
> Subject: [PATCH] add_delta_base_cache: use list_for_each_safe
>
> We may remove elements from the list while we are iterating,
> which requires using a second temporary pointer. Otherwise
> stepping to the next element of the list might involve
> looking at freed memory (which generally works in practice,
> as we _just_ freed it, but of course is wrong to rely on;
> valgrind notices it).
I failed to notice it, too. Thanks.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> sha1_file.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index a57b71d..132c861 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2187,11 +2187,11 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,
> void *base, unsigned long base_size, enum object_type type)
> {
> struct delta_base_cache_entry *ent = xmalloc(sizeof(*ent));
> - struct list_head *lru;
> + struct list_head *lru, *tmp;
>
> delta_base_cached += base_size;
>
> - list_for_each(lru, &delta_base_cache_lru) {
> + list_for_each_safe(lru, tmp, &delta_base_cache_lru) {
> struct delta_base_cache_entry *f =
> list_entry(lru, struct delta_base_cache_entry, lru);
> if (delta_base_cached <= delta_base_cache_limit)
^ permalink raw reply
* Re: [PATCH] t/perf/run: Don't forget to copy config.mak.autogen & friends to build area
From: Junio C Hamano @ 2016-09-12 19:12 UTC (permalink / raw)
To: Kirill Smelkov
Cc: Jeff King, Vicent Marti, Jérome Perrin, Isabelle Vallet,
Kazuhiko Shiozaki, Julien Muchembled, git
In-Reply-To: <20160910150512.19473-1-kirr@nexedi.com>
Kirill Smelkov <kirr@nexedi.com> writes:
> Otherwise for people who use autotools-based configure in main worktree,
> the performance testing results will be inconsistent as work and build
> trees could be using e.g. different optimization levels.
>
> See e.g.
>
> http://public-inbox.org/git/20160818175222.bmm3ivjheokf2qzl@sigill.intra.peff.net/
>
> for example.
>
> NOTE config.status has to be copied because otherwise without it the build
> would want to run reconfigure this way loosing just copied config.mak.autogen.
>
> Signed-off-by: Kirill Smelkov <kirr@nexedi.com>
> ---
> ( Resending as separate patch-mail, just in case )
>
> t/perf/run | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/perf/run b/t/perf/run
> index cfd7012..aa383c2 100755
> --- a/t/perf/run
> +++ b/t/perf/run
> @@ -30,7 +30,7 @@ unpack_git_rev () {
> }
> build_git_rev () {
> rev=$1
> - cp ../../config.mak build/$rev/config.mak
> + cp -t build/$rev ../../{config.mak,config.mak.autogen,config.status}
That unfortunately is a GNUism -t with a bash-ism {a,b,c}; just keep
it simple and stupid to make sure it is portable.
This is not even a part that we measure the runtime for anyway.
> (cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||
> die "failed to build revision '$mydir'"
> }
^ permalink raw reply
* Re: [PATCH] t/perf/run: Don't forget to copy config.mak.autogen & friends to build area
From: Junio C Hamano @ 2016-09-12 19:17 UTC (permalink / raw)
To: Kirill Smelkov
Cc: Jeff King, Vicent Marti, Jérome Perrin, Isabelle Vallet,
Kazuhiko Shiozaki, Julien Muchembled, git
In-Reply-To: <xmqqh99l2aqt.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> build_git_rev () {
>> rev=$1
>> - cp ../../config.mak build/$rev/config.mak
>> + cp -t build/$rev ../../{config.mak,config.mak.autogen,config.status}
>
> That unfortunately is a GNUism -t with a bash-ism {a,b,c}; just keep
> it simple and stupid to make sure it is portable.
>
> This is not even a part that we measure the runtime for anyway.
In other words, something along this line, perhaps.
t/perf/run | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/t/perf/run b/t/perf/run
index aa383c2..69a4714 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -30,7 +30,10 @@ unpack_git_rev () {
}
build_git_rev () {
rev=$1
- cp -t build/$rev ../../{config.mak,config.mak.autogen,config.status}
+ for config in config.mak config.mak.autogen config.status
+ do
+ cp "../../$config" "build/$rev/"
+ done
(cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||
die "failed to build revision '$mydir'"
}
--
2.10.0-342-gc678130
^ permalink raw reply related
* Re: [PATCH 2/2 v7] pack-objects: use reachability bitmap index when generating non-stdout pack
From: Junio C Hamano @ 2016-09-12 19:21 UTC (permalink / raw)
To: Kirill Smelkov
Cc: Jeff King, Vicent Marti, Jérome Perrin, Isabelle Vallet,
Kazuhiko Shiozaki, Julien Muchembled, git
In-Reply-To: <20160910145925.xbbus7eck5ineika@teco.navytux.spb.ru>
Kirill Smelkov <kirr@nexedi.com> writes:
>> This is v7, but as I understand your numbering, it goes with v5 of patch
>> 1/2 that I just reviewed (usually we just increment the version number
>> on the whole series and treat it as a unit, even if some patches didn't
>> change from version to version).
>
> The reason those patches are having their own numbers is that they are
> orthogonal to each other and can be applied / rejected independently.
In such a case, we wouldn't label them 1/2 and 2/2, which tells the
readers that these are two pieces that are to be applied together to
form a single unit of change. That was what these numbered patches
with different version numbers confusing.
> But ok, since now we have them considered both together, their next
> versions posted will be uniform v8.
OK. Thanks for clarifying.
^ permalink raw reply
* build issues on AIX - aka non-GNU environment, no gnu grep, no gcc
From: Michael Felt @ 2016-09-12 19:28 UTC (permalink / raw)
To: git
I had a couple of issues when packaging git for AIX
a) option -Wall by default - works fine with gcc I am sure, but not so
well when gcc is not your compiler
b) needs a special (GNU) grep argument (-a from memory). This I
resolved by downloading and packaging GNU grep. However, I hope this
has not introduced a new dependency.
FYI: as I recall, the last time I build git (2.6.4 I believe) I had
neither of these problems (although I did later find I had once upon a
time packaged, just never published grep-2.22)
Also, if you wish to add my aixtools as a place to get git packaged
for AIX - just get in touch and we can exchange details.
Michael
^ permalink raw reply
* Re: [PATCH v2 2/4] update-index: use the same structure for chmod as add
From: Thomas Gummerer @ 2016-09-12 19:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Jeff King, Jan Keromnes,
Ingo Brückl, Edward Thomson
In-Reply-To: <xmqqinu26ph0.fsf@gitster.mtv.corp.google.com>
On 09/11, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > @@ -955,10 +941,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> > PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
> > PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> > (parse_opt_cb *) cacheinfo_callback},
> > - {OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
> > - N_("override the executable bit of the listed files"),
> > - PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> > - chmod_callback},
> > + OPT_STRING( 0, "chmod", &chmod_arg, N_("(+/-)x"),
> > + N_("override the executable bit of the listed files")),
> > {OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
> > N_("mark files as \"not changing\""),
> > PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
> > @@ -1018,6 +1002,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> > if (argc == 2 && !strcmp(argv[1], "-h"))
> > usage_with_options(update_index_usage, options);
> >
> > + if (!chmod_arg)
> > + force_mode = 0;
> > + else if (!strcmp(chmod_arg, "-x"))
> > + force_mode = 0666;
> > + else if (!strcmp(chmod_arg, "+x"))
> > + force_mode = 0777;
> > + else
> > + die(_("option 'chmod' expects \"+x\" or \"-x\""));
> > +
>
> I am afraid that this changes the behaviour drastically.
>
> "git update-index" is an oddball command that takes options and then
> processes them immediately, exactly because it was designed to take
>
> git update-index --chmod=-x A --chmod=+x B --add C
>
> and say things like "A and B are not in the index and you are
> attempting to add them before giving me --add option".
>
> git update-index --add --chmod=-x A --chmod=+x B C
>
> is expected to add A as non-executable, and B and C as executable.
> Many exotic parse-options callback mechanisms used in this command
> were invented exactly to support its quirky way of not doing "get a
> list of options and use the last one". And this patch breaks it for
> only one option without changing the others.
>
> If we were willing to take such a big backward compatiblity hit in
> the upcoming release (which I personally won't be affected, but old
> scripts by others need to be audited and adjusted, which I won't
> volunteer to do myself), we should make such a change consistently,
> e.g. "git update-index A --add --remove B" should no longer error
> out when it sees A and it is not yet in the index because "--add"
> hasn't been given yet, or A is in the index but is missing from the
> working tree because "--remove" hasn't been given yet. Then it may
> be more justifiable if "update-index --chmod=-x A --chmod=+x B"
> added A as an executable. With the current form of this patch, it
> is not.
Thanks for the explanation, this change in backwards compatibility is
certainly not what I intended, but rather something I missed while
cooking up this patch.
> Can we do this "fix" without this change?
Yeah, let me see what I can come up with in a re-roll.
Thanks,
Thomas
^ permalink raw reply
* Re: Gitattributes file is not respected when switching between branches
From: Torsten Bögershausen @ 2016-09-12 19:35 UTC (permalink / raw)
To: Виталий Ищенко,
git
In-Reply-To: <CANYoZJng0GNZWU=eUEnXgVQ_NKQQOKM+mhJ9bsXMEJxxEhwQMw@mail.gmail.com>
On 12.09.16 14:55, Виталий Ищенко wrote:
> Good day
>
> I faced following issue with gitattributes file (at least eol setting)
> when was trying to force `lf` mode on windows.
>
> We have 2 branches: master & dev. With master set as HEAD in repository
>
> I've added `.gitattributes` with following content to `dev` branch
>
> ```
> * text eol=lf
> ```
>
> Now when you clone this repo on other machine and checkout dev branch,
> eol setting is not respected.
> As a workaround you can rm all files except .git folder and do hard reset.
>
> Issue is reproducible on windows & unix versions. Test repo can be
> found on github
> https://github.com/betalb/gitattributes-issue
>
> master branch - one file without gitattributes
> feature-branch - .gitattributes added with eol=lf
> unix-feature-branch - .gitattributes added with eol=crlf
>
> Thanks,
> Vitalii
Some more information may be needed, to help to debug.
Which version of Git are you using ?
What does
git ls-files --eol
say ?
^ permalink raw reply
* Re: [PATCH v2 0/5] Pull out require_clean_work_tree() functionality from builtin/pull.c
From: Junio C Hamano @ 2016-09-12 19:36 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <cover.1473580914.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Johannes Schindelin (5):
> pull: drop confusing prefix parameter of die_on_unclean_work_tree()
> pull: make code more similar to the shell script again
> Make the require_clean_work_tree() function truly reusable
> Export also the has_un{staged,committed}_changed() functions
> wt-status: teach has_{unstaged,uncommitted}_changes() about submodules
Other than two minor things I've already mentioned, this round looks
ready to be queued. Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox