* Re: [PATCH v2] submodule: add 'deinit' command
From: Marc Branchaud @ 2012-12-13 15:47 UTC (permalink / raw)
To: Jens Lehmann
Cc: Junio C Hamano, Michael J Gruber, Phil Hord, W. Trevor King, Git,
Heiko Voigt, Jeff King, Shawn Pearce, Nahor
In-Reply-To: <50C90469.8080303@web.de>
On 12-12-12 05:25 PM, Jens Lehmann wrote:
>
> So unless people agree that deinit should also remove the work
> tree I'll prepare some patches teaching all git commands to
> consistently ignore deinitialized submodules. Opinions?
I agree with Trevor's suggestion that deinit should restore the user to the
state he would be in if he were never interested in the submodule. So clean
up .git/config and remove the work tree. (Maybe just issue a warning instead
if the submodule's work tree is dirty.)
Also, given that semantic, I agree with Michael that a bare "git submodule
deinit" should *not* deinitialize all the submodules. It should require a
"--all" for that. The bare command should just print a usage summary.
M.
^ permalink raw reply
* RE: FW: Git log --graph doesn't output color when redirected
From: Srb, Michal @ 2012-12-13 15:34 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org
In-Reply-To: <20121213131329.GA5042@sigill.intra.peff.net>
From: Jeff King [peff@peff.net]
Sent: Thursday, December 13, 2012 1:13 PM
>> Is there a setting somewhere in config to change this?
> Yes. If you use "--color" on the command line, that means
> "unconditionally use color". If you set color.ui (or any other
> color config option) to "always", then you will always get color (and
> you can disable it for a particular run with "--no-color"). Setting a color
> config option to "true" is the same as "auto", which gets you
> the auto mode.
Setting color in gitconfig didn't work for me on msys, but --color
works like magic, thanks!
^ permalink raw reply
* Build fixes for another obscure Unix
From: David Michael @ 2012-12-13 15:22 UTC (permalink / raw)
To: git
Hi,
I've been experimenting with git running on z/OS USS. It is not yet
stable, but I have had to make a few fixes and generalizations in the
build system to get it to compile.
Would there be any interest in applying such individual compatibility
fixes for this system, even if a full port doesn't reach completion?
Thanks.
David
^ permalink raw reply
* [PATCH] Fix sizeof usage in get_permutations
From: Matthew Daley @ 2012-12-13 13:36 UTC (permalink / raw)
To: git; +Cc: Matthew Daley
Currently it gets the size of an otherwise unrelated, unused variable
instead of the expected struct size.
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
builtin/pack-redundant.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index f5c6afc..7544687 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -301,14 +301,14 @@ static void pll_free(struct pll *l)
*/
static struct pll * get_permutations(struct pack_list *list, int n)
{
- struct pll *subset, *ret = NULL, *new_pll = NULL, *pll;
+ struct pll *subset, *ret = NULL, *new_pll = NULL;
if (list == NULL || pack_list_size(list) < n || n == 0)
return NULL;
if (n == 1) {
while (list) {
- new_pll = xmalloc(sizeof(pll));
+ new_pll = xmalloc(sizeof(struct pll));
new_pll->pl = NULL;
pack_list_insert(&new_pll->pl, list);
new_pll->next = ret;
@@ -321,7 +321,7 @@ static struct pll * get_permutations(struct pack_list *list, int n)
while (list->next) {
subset = get_permutations(list->next, n - 1);
while (subset) {
- new_pll = xmalloc(sizeof(pll));
+ new_pll = xmalloc(sizeof(struct pll));
new_pll->pl = subset->pl;
pack_list_insert(&new_pll->pl, list);
new_pll->next = ret;
--
1.7.10.4
^ permalink raw reply related
* Re: FW: Git log --graph doesn't output color when redirected
From: Jeff King @ 2012-12-13 13:13 UTC (permalink / raw)
To: Srb, Michal; +Cc: git@vger.kernel.org
In-Reply-To: <72BB37CB88C48F4B925365539F1EE46C18261403@icexch-m3.ic.ac.uk>
On Wed, Dec 12, 2012 at 05:35:17PM +0000, Srb, Michal wrote:
> Unlike --pretty-format, --graph doesn’t output colors when the git log output
> is redirected.
I do not think it has anything to do with --graph in particular, but
rather that when colorization is set to the "auto" mode, it is enabled
only when stdout is a tty. Diff coloring, for example, follows the same
rules. If you are using --format="%C(red)" or similar placeholders,
they are the odd duck by not respecting the auto-color mode.
> Is there a setting somewhere in config to change this?
Yes. If you use "--color" on the command line, that means
"unconditionally use color". If you set color.ui (or any other color
config option) to "always", then you will always get color (and you can
disable it for a particular run with "--no-color"). Setting a color
config option to "true" is the same as "auto", which gets you the auto
mode.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] mailmap: support reading mailmap from blobs
From: Jeff King @ 2012-12-13 13:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20121212110404.GB19653@sigill.intra.peff.net>
On Wed, Dec 12, 2012 at 06:04:04AM -0500, Jeff King wrote:
> In a bare repository, there isn't a simple way to respect an
> in-tree mailmap without extracting it to a temporary file.
> This patch provides a config variable, similar to
> mailmap.file, which reads the mailmap from a blob in the
> repository.
While preparing the "default mailmap.blob" patch, I noticed a few loose
ends in the documentation. They can be squashed into this 2/2
(jk/mailmap-from-blob~1), or can go on top of the series:
-- >8 --
Subject: [PATCH] fix some documentation loose-ends for mailmap.blob
Anywhere we mention mailmap.file, it is probably worth
mentioning mailmap.blob, as well.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/git-log.txt | 2 +-
Documentation/mailmap.txt | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 585dac4..08a185d 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -167,7 +167,7 @@ log.showroot::
`git log -p` output would be shown without a diff attached.
The default is `true`.
-mailmap.file::
+mailmap.*::
See linkgit:git-shortlog[1].
notes.displayRef::
diff --git a/Documentation/mailmap.txt b/Documentation/mailmap.txt
index 288f04e..bb349c2 100644
--- a/Documentation/mailmap.txt
+++ b/Documentation/mailmap.txt
@@ -1,5 +1,6 @@ If the file `.mailmap` exists at the toplevel of the repository, or at
If the file `.mailmap` exists at the toplevel of the repository, or at
-the location pointed to by the mailmap.file configuration option, it
+the location pointed to by the mailmap.file or mailmap.blob
+configuration options, it
is used to map author and committer names and email addresses to
canonical real names and email addresses.
--
1.8.0.2.4.g59402aa
^ permalink raw reply related
* Re: [PATCH 0/2] mailmap from blobs
From: Jeff King @ 2012-12-13 13:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20121212175900.GA32767@sigill.intra.peff.net>
On Wed, Dec 12, 2012 at 12:59:00PM -0500, Jeff King wrote:
> > Have you considered defaulting to read from HEAD:.mailmap even when
> > this new configuration is not there if core.bare is set? I would
> > imagine that it would be the most convenient and match people's
> > expectations.
>
> Yeah, I almost suggested that, but I figured it could wait for the
> feature to prove itself in the real world before turning it on by
> default. It _should_ be pretty harmless, though, so I don't mind turning
> it on by default.
That patch would look like this:
-- >8 --
Subject: [PATCH] mailmap: default mailmap.blob in bare repositories
The motivation for mailmap.blob is to let users of bare
repositories use the mailmap feature, as they would not have
a checkout containing the .mailmap file. We can make it even
easier for them by just looking in HEAD:.mailmap by default.
We can't know for sure that this is where they would keep a
mailmap, of course, but it is the best guess (and it matches
the non-bare behavior, which reads from HEAD:.mailmap in the
working tree). If it's missing, git will silently ignore the
setting.
We do not do the same magic in the non-bare case, because:
1. In the common case, HEAD:.mailmap will be the same as
the .mailmap in the working tree, which is a no-op.
2. In the uncommon case, the user has modified .mailmap
but not yet committed it, and would expect the working
tree version to take precedence.
Signed-off-by: Jeff King <peff@peff.net>
---
I went with defaulting mailmap.blob, because it provides an easy path
for turning off the feature (you just override mailmap.blob).
Another way of thinking about this would be that it is the bare analog
to "read .mailmap from the working tree", and the logic should be:
if (is_bare_repository())
read_mailmap_blob(map, "HEAD:.mailmap");
else
read_mailmap_file(map, ".mailmap");
read_mailmap_blob(map, git_mailmap_blob);
read_mailmap_file(map, git_mailmap_file);
However, the current is not exactly "read from the root of the working
tree". It is "read from the current directory", and it works because all
of the callers run from the toplevel of the working tree (when one
exists). That means that bare repositories have always read from
$GIT_DIR/.mailmap. I doubt that was intentional, but people with bare
repositories may be depending on it. So I think that falls into the
"how I might do it from scratch" bin.
We could still do:
if (is_bare_repository())
read_mailmap_blob(map, "HEAD:.mailmap");
read_mailmap_file(map, ".mailmap");
read_mailmap_blob(map, git_mailmap_blob);
read_mailmap_file(map, git_mailmap_file);
but IMHO that is just making the rules more complicated to explain for
little benefit (and in fact, you lose the ability to suppress the HEAD
mailmap, which you might care about if you are hosting multiple bits of
unrelated history in the same repo).
Documentation/config.txt | 8 +++++---
mailmap.c | 5 +++++
t/t4203-mailmap.sh | 25 +++++++++++++++++++++++++
3 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3760077..1a3c554 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1519,9 +1519,11 @@ mailmap.blob::
mailmap.blob::
Like `mailmap.file`, but consider the value as a reference to a
- blob in the repository (e.g., `HEAD:.mailmap`). If both
- `mailmap.file` and `mailmap.blob` are given, both are parsed,
- with entries from `mailmap.file` taking precedence.
+ blob in the repository. If both `mailmap.file` and
+ `mailmap.blob` are given, both are parsed, with entries from
+ `mailmap.file` taking precedence. In a bare repository, this
+ defaults to `HEAD:.mailmap`. In a non-bare repository, it
+ defaults to empty.
man.viewer::
Specify the programs that may be used to display help in the
diff --git a/mailmap.c b/mailmap.c
index 5ffe48a..b16542f 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -233,7 +233,12 @@ int read_mailmap(struct string_list *map, char **repo_abbrev)
int read_mailmap(struct string_list *map, char **repo_abbrev)
{
int err = 0;
+
map->strdup_strings = 1;
+
+ if (!git_mailmap_blob && is_bare_repository())
+ git_mailmap_blob = "HEAD:.mailmap";
+
err |= read_mailmap_file(map, ".mailmap", repo_abbrev);
err |= read_mailmap_blob(map, git_mailmap_blob, repo_abbrev);
err |= read_mailmap_file(map, git_mailmap_file, repo_abbrev);
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index e7ea40c..aae30d9 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -218,6 +218,31 @@ test_expect_success 'mailmap.blob can be missing' '
test_cmp expect actual
'
+test_expect_success 'mailmap.blob defaults to off in non-bare repo' '
+ git init non-bare &&
+ (
+ cd non-bare &&
+ test_commit one .mailmap "Fake Name <author@example.com>" &&
+ echo " 1 Fake Name" >expect &&
+ git shortlog -ns HEAD >actual &&
+ test_cmp expect actual &&
+ rm .mailmap &&
+ echo " 1 A U Thor" >expect &&
+ git shortlog -ns HEAD >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'mailmap.blob defaults to HEAD:.mailmap in bare repo' '
+ git clone --bare non-bare bare &&
+ (
+ cd bare &&
+ echo " 1 Fake Name" >expect &&
+ git shortlog -ns HEAD >actual &&
+ test_cmp expect actual
+ )
+'
+
test_expect_success 'cleanup after mailmap.blob tests' '
rm -f .mailmap
'
--
1.8.0.2.4.g59402aa
^ permalink raw reply related
* Re: [PATCH] git(1): remove a defunct link to "list of authors"
From: Jeff King @ 2012-12-13 12:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyen Thai Ngoc Duy, git
In-Reply-To: <7v8v935en3.fsf@alter.siamese.dyndns.org>
On Wed, Dec 12, 2012 at 10:06:24AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I find the ohloh one a little more informative than the GitHub graph. I
> > couldn't find any others (Google Code does not seem to have one,
> > kernel.org and other gitweb sites do not, and I can't think of anywhere
> > else that hosts a mirror).
>
> Then let's do this.
>
> -- >8 --
> Subject: git(1): show link to contributor summary page
>
> We earlier removed a link to list of contributors that pointed to a
> defunct page; let's use a working one from Ohloh.net to replace it
> instead.
Looks good to me. Thanks.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2012, #03; Wed, 12)
From: Max Horn @ 2012-12-13 12:04 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Junio C Hamano, git
In-Reply-To: <CAMP44s3uyC0V6ycTv78mG36_i7ugMLwwNk2cqNZatEJuL7Ee1w@mail.gmail.com>
On 13.12.2012, at 11:08, Felipe Contreras wrote:
> On Thu, Dec 13, 2012 at 2:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>>> New remote helper for bzr (v3). With minor fixes, this may be ready
>>>> for 'next'.
>>>
>>> What minor fixes?
>>
>> Lookng at the above (fixup), $gmane/210744 comes to mind
>
> That doesn't matter. The code and the tests would work just fine.
It doesn't matter? I find that statement hard to align with what the maintainer of git, and thus the person who decides whether your patch series gets merged or not, wrote just above? In fact, it seems to me that what Junio said matters a great deal...
This is a very strange attitude...
In another email, you complained about nobody reviewing your patches respectively nobody voicing any constructive criticism. Yet Junio did just that, and again in $gmane/210745 -- and you replied to neither, and acted on neither (not even by refuting the points brought up), and now summarily dismiss them as irrelevant. I find that quite disturbing :-(.
>
>> but there may be others. It is the responsibility of a contributor to keep
>> track of review comments others give to his or her patches and
>> reroll them, so I do not recall every minor details, sorry.
>
> There is nothing that prevents remote-bzr from being merged.
Well, I think that is up to Junio to decide in the end, though :-). He wrote
Cheers,
Max
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2012, #03; Wed, 12)
From: Felipe Contreras @ 2012-12-13 10:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vvcc6z801.fsf@alter.siamese.dyndns.org>
On Thu, Dec 13, 2012 at 2:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>> New remote helper for bzr (v3). With minor fixes, this may be ready
>>> for 'next'.
>>
>> What minor fixes?
>
> Lookng at the above (fixup), $gmane/210744 comes to mind
That doesn't matter. The code and the tests would work just fine.
> but there may be others. It is the responsibility of a contributor to keep
> track of review comments others give to his or her patches and
> reroll them, so I do not recall every minor details, sorry.
There is nothing that prevents remote-bzr from being merged.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2012, #03; Wed, 12)
From: Junio C Hamano @ 2012-12-13 8:11 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git
In-Reply-To: <CAMP44s2DAuhk5FkDm0-cYsikY0o6vuZ4FyAnXhbtsgqKQF1dpg@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> On Wed, Dec 12, 2012 at 5:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> [Stalled]
>>
>> * fc/remote-bzr (2012-11-28) 10 commits
>> - (fixup) test-bzr.sh: fix multi-line string assignment
>> - remote-bzr: detect local repositories
>> - remote-bzr: add support for older versions of bzr
>> - remote-bzr: add support to push special modes
>> - remote-bzr: add support for fecthing special modes
>> - remote-bzr: add simple tests
>> - remote-bzr: update working tree
>> - remote-bzr: add support for remote repositories
>> - remote-bzr: add support for pushing
>> - Add new remote-bzr transport helper
>>
>> New remote helper for bzr (v3). With minor fixes, this may be ready
>> for 'next'.
>
> What minor fixes?
Lookng at the above (fixup), $gmane/210744 comes to mind, but there
may be others. It is the responsibility of a contributor to keep
track of review comments others give to his or her patches and
reroll them, so I do not recall every minor details, sorry.
^ permalink raw reply
* unclear documentation of git fetch --tags option and tagopt config
From: 乙酸鋰 @ 2012-12-13 6:29 UTC (permalink / raw)
To: git
Hi,
With git fetch --tags
or remote.origin.tagopt = --tags
git fetch only fetches tags, but not branches.
Current documentation does not mention that no branches are fetched /
pulled when --tags option or remote.origin.tagopt = --tags is
specified.
Regards,
ch3cooli
^ permalink raw reply
* Re: possible Improving diff algoritm
From: Junio C Hamano @ 2012-12-13 6:26 UTC (permalink / raw)
To: Geert Bosch; +Cc: Morten Welinder, Kevin, git
In-Reply-To: <B1564B28-9BB9-48A2-B59E-7D7C0B0DDECF@adacore.com>
Geert Bosch <bosch@adacore.com> writes:
> It would seem that just looking at the line length (stripped) of
> the last line, might be sufficient for cost function to minimize.
> Here the some would be 3 vs 0. In case of ties, use the last
> possibility with minimum cost.
-- 8< --
#ifdef A
some stuff
about A
#endif
#ifdef Z
some more stuff
about Z
#endif
-- >8 --
If you insert a block for M following the existing formatting
convention in the middle, your heuristics will pick the blank line
after "about A" as having minimum cost, no?
You inherently have to know the nature of the payload, as your eyes
that judge the result use that knowledge when doing so, I am afraid.
I think your "define a function that gives a good score to lines
that are likely to be good breaking points" idea has merit, but I
think that should be tied to the content type, most likely via the
attribute mechanism.
In any case, I consider this as a low-impact (as Michael Haggerty
noted, it is impossible to introduce a bug that subtly break the
output; your result is either totally borked or is correct) and
low-hanging fruit (it can be done as a postprocessing phase after
the xdiff machinery has done the heavy-lifting of computing LCA), if
somebody wants to experiment and implement one. As long as the new
heuristics is hidden behind an explicit command line option to avoid
other "consequences", I wouldn't discourage interested parties from
working on it. It is not just my itch, though.
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2012, #03; Wed, 12)
From: Felipe Contreras @ 2012-12-13 6:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vhanq257s.fsf@alter.siamese.dyndns.org>
On Wed, Dec 12, 2012 at 5:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> [Stalled]
>
> * fc/remote-bzr (2012-11-28) 10 commits
> - (fixup) test-bzr.sh: fix multi-line string assignment
> - remote-bzr: detect local repositories
> - remote-bzr: add support for older versions of bzr
> - remote-bzr: add support to push special modes
> - remote-bzr: add support for fecthing special modes
> - remote-bzr: add simple tests
> - remote-bzr: update working tree
> - remote-bzr: add support for remote repositories
> - remote-bzr: add support for pushing
> - Add new remote-bzr transport helper
>
> New remote helper for bzr (v3). With minor fixes, this may be ready
> for 'next'.
What minor fixes?
--
Felipe Contreras
^ permalink raw reply
* Submodule not updated automatically on merge conflict
From: 乙酸鋰 @ 2012-12-13 5:46 UTC (permalink / raw)
To: git
Hi,
If there are merge conflict files, then changed submodules are not
updated automatically.
Why not submodules?
Files do try to merge / update.
Regards,
ch3cooli
^ permalink raw reply
* Re: possible Improving diff algoritm
From: Geert Bosch @ 2012-12-13 4:58 UTC (permalink / raw)
To: Morten Welinder; +Cc: Junio C Hamano, Kevin, git
In-Reply-To: <CANv4PNnC1J54TSpHuBOpY=rbuU_naysYkmoyi=utNF0vWK1CnA@mail.gmail.com>
On Dec 12, 2012, at 20:55, Morten Welinder <mwelinder@gmail.com> wrote:
> I was merely asking if an algorithm to pick between the
> 2+ choices was allowed to look at the contents of the
> lines.
>
> I.e., an algorithm would look at the C comment
> example and determine that the choice starting containing
> a full inserted comment is preferable over the one that
> appears to close one comment and open a new.
>
> And the in inserted-function case it would prefer the one
> where the matching { and } are in correct order.
/** + /**
+ * Default parent + * Default parent
+ * + *
+ * @var int + * @var int
+ * @access protected + * @access protected
+ * @index + * @index
+ */ + */
+ protected $defaultParent; + protected $defaultParent;
+ +
+ /** /**
It would seem that just looking at the line length (stripped) of
the last line, might be sufficient for cost function to minimize.
Here the some would be 3 vs 0. In case of ties, use the last
possibility with minimum cost.
I think it would be nice if the cost function we choose does not
depend on file type, as that is something that is very dependent
on the exact local configuration and might hinder comparison of
patches. If something really simple gets us 90% there, that would
be preferable over extra complexity.
-Geert
Junio's other example:
}
+void new_function(void)
+{
+ printf("hello, world.\n");
+}
+
void existing_one(void)
{
printf("goodbye, world.\n");
=> Cost 0
+}
+
+void new_function(void)
+{
+ printf("hello, world.\n");
}
=> Cost 27
Kevin's example:
/**
+ * Default parent
+ *
+ * @var int
+ * @access protected
+ * @index
+ */
+ protected $defaultParent;
+
+ /**
=> Cost 3
+ /**
+ * Default parent
+ *
+ * @var int
+ * @access protected
+ * @index
+ */
+ protected $defaultParent;
+
/**
=> cost 0
^ permalink raw reply
* Re: [PATCH v3 1/2] cache-tree: invalidate i-t-a paths after generating trees
From: Junio C Hamano @ 2012-12-13 2:04 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, Jonathon Mah
In-Reply-To: <1355362747-13474-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Intent-to-add entries used to forbid writing trees so it was not a
> problem. After commit 3f6d56d (commit: ignore intent-to-add entries
> instead of refusing - 2012-02-07), we can generate trees from an index
> with i-t-a entries.
>
> However, the commit forgets to invalidate all paths leading to i-t-a
> entries. With fully valid cache-tree (e.g. after commit or
> write-tree), diff operations may prefer cache-tree to index and not
> see i-t-a entries in the index, because cache-tree does not have them.
>
> Reported-by: Jonathon Mah <me@JonathonMah.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> This version ensures that entry_count can only be >= -1 after
> update_one returns. Not ideal but good enough.
>
> cache-tree.c | 40 ++++++++++++++++++++++++++++++++++++----
> t/t2203-add-intent.sh | 20 ++++++++++++++++++++
> 2 files changed, 56 insertions(+), 4 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 28ed657..1fbc81a 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -248,6 +248,7 @@ static int update_one(struct cache_tree *it,
> int missing_ok = flags & WRITE_TREE_MISSING_OK;
> int dryrun = flags & WRITE_TREE_DRY_RUN;
> int i;
> + int to_invalidate = 0;
>
> if (0 <= it->entry_count && has_sha1_file(it->sha1))
> return it->entry_count;
> @@ -324,7 +325,14 @@ static int update_one(struct cache_tree *it,
> if (!sub)
> die("cache-tree.c: '%.*s' in '%s' not found",
> entlen, path + baselen, path);
> - i += sub->cache_tree->entry_count - 1;
> + i--; /* this entry is already counted in "sub" */
Huh?
The "-1" in the original is the bog-standard compensation for the
for(;;i++) loop.
> + if (sub->cache_tree->entry_count < 0) {
> + i -= sub->cache_tree->entry_count;
> + sub->cache_tree->entry_count = -1;
> + to_invalidate = 1;
> + }
> + else
> + i += sub->cache_tree->entry_count;
While the rewritten version is not *wrong* per-se, I have a feeling
that it may be much easier to read if written like this:
if (sub->cache_tree_entry_count < 0) {
to_invalidate = 1;
to_skip = 0 - sub->cache_tree_entry_count;
sub->cache_tree_entry_count = -1;
} else {
to_skip = sub->cache_tree_entry_count;
}
i += to_skip - 1;
> @@ -360,7 +383,7 @@ static int update_one(struct cache_tree *it,
> }
>
> strbuf_release(&buffer);
> - it->entry_count = i;
> + it->entry_count = to_invalidate ? -i : i;
> #if DEBUG
> fprintf(stderr, "cache-tree update-one (%d ent, %d subtree) %s\n",
> it->entry_count, it->subtree_nr,
> @@ -381,6 +404,15 @@ int cache_tree_update(struct cache_tree *it,
> i = update_one(it, cache, entries, "", 0, flags);
> if (i < 0)
> return i;
> + /*
> + * update_one() uses negative entry_count as a way to mark an
> + * entry invalid _and_ pass the number of entries back to
> + * itself at the parent level. This is for internal use and
> + * should not be leaked out after the top-level update_one
> + * exits.
> + */
> + if (it->entry_count < 0)
> + it->entry_count = -1;
Nice. I think what entry_count means immediately after update_one()
returned should be commented near the beginning of that function,
too, though.
Thanks.
^ permalink raw reply
* Re: Fwd: possible Improving diff algoritm
From: Morten Welinder @ 2012-12-13 1:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kevin, git
In-Reply-To: <7vpq2f2az4.fsf@alter.siamese.dyndns.org>
>> Is there a reason why picking among the choices in a sliding window
>> must be contents neutral?
>
> Sorry, you might be getting at something interesting but I do not
> understand the question. I have no idea what you mean by "contents
> neutral".
I was merely asking if an algorithm to pick between the
2+ choices was allowed to look at the contents of the
lines.
I.e., an algorithm would look at the C comment
example and determine that the choice starting containing
a full inserted comment is preferable over the one that
appears to close one comment and open a new.
And the in inserted-function case it would prefer the one
where the matching { and } are in correct order.
Morten
^ permalink raw reply
* Re: [PATCH v2] index-format.txt: be more liberal on what can represent invalid cache tree
From: Junio C Hamano @ 2012-12-13 1:55 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1355361287-10875-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> How would that work with existing versions? If you write -2 in
> cache-tree, the next time 1.8.0 updates cache tree it writes -1 back.
> That loses whatever information you attach to -2. A new cache-tree
> extension is probably better.
You can easily imagine a definition like this:
- If non-negative, the entry is valid and it is the number of index
entries that are covered by this subtree;
- If -1, the cached-tree does not know the object name of the tree
object, and nothing else is known. The caller needs to walk the
index to skip the entries that could have been covered by this
subtree, if the cached tree information were valid;
- If less than -1, the cached-tree does not know the object name of
the tree object, but we know the number of index entries that are
covered by this subtree. The caller, instead of walking the index,
can subtract the count from -1 and skip that many entries to find
the index entry after the part that is inside this directory.
Newer Git may write -201 and newer Git may be able to take advantage
of the new information encoded there, saving 200 calls to
prefixcmp(), while given the same index, older Git will operate just
as correctly as before. An older Git may write that part of the
cache-tree back with -1, defeating the optimization when a newer Git
reads it back, but by definition the older Git does not know what to
write to help that optimization that did not exist when it was done,
and writing -1 will not confuse the newer Git.
^ permalink raw reply
* [PATCH v3 2/2] cache-tree: remove dead i-t-a code in verify_cache()
From: Nguyễn Thái Ngọc Duy @ 2012-12-13 1:39 UTC (permalink / raw)
To: git
Cc: Jeff King, Junio C Hamano, Jonathon Mah,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1355362747-13474-1-git-send-email-pclouds@gmail.com>
This code is added in 331fcb5 (git add --intent-to-add: do not let an
empty blob be committed by accident - 2008-11-28) to forbid committing
when i-t-a entries are present. When we allow that, we forgot to
remove this unreachable code.
Noticed-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
cache-tree.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/cache-tree.c b/cache-tree.c
index 1fbc81a..3e79e27 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -166,12 +166,8 @@ static int verify_cache(struct cache_entry **cache,
fprintf(stderr, "...\n");
break;
}
- if (ce_stage(ce))
- fprintf(stderr, "%s: unmerged (%s)\n",
- ce->name, sha1_to_hex(ce->sha1));
- else
- fprintf(stderr, "%s: not added yet\n",
- ce->name);
+ fprintf(stderr, "%s: unmerged (%s)\n",
+ ce->name, sha1_to_hex(ce->sha1));
}
}
if (funny)
--
1.8.0.rc2.23.g1fb49df
^ permalink raw reply related
* [PATCH v3 1/2] cache-tree: invalidate i-t-a paths after generating trees
From: Nguyễn Thái Ngọc Duy @ 2012-12-13 1:39 UTC (permalink / raw)
To: git
Cc: Jeff King, Junio C Hamano, Jonathon Mah,
Nguyễn Thái Ngọc Duy
In-Reply-To: <7vip89bz4v.fsf@alter.siamese.dyndns.org>
Intent-to-add entries used to forbid writing trees so it was not a
problem. After commit 3f6d56d (commit: ignore intent-to-add entries
instead of refusing - 2012-02-07), we can generate trees from an index
with i-t-a entries.
However, the commit forgets to invalidate all paths leading to i-t-a
entries. With fully valid cache-tree (e.g. after commit or
write-tree), diff operations may prefer cache-tree to index and not
see i-t-a entries in the index, because cache-tree does not have them.
Reported-by: Jonathon Mah <me@JonathonMah.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
This version ensures that entry_count can only be >= -1 after
update_one returns. Not ideal but good enough.
cache-tree.c | 40 ++++++++++++++++++++++++++++++++++++----
t/t2203-add-intent.sh | 20 ++++++++++++++++++++
2 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/cache-tree.c b/cache-tree.c
index 28ed657..1fbc81a 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -248,6 +248,7 @@ static int update_one(struct cache_tree *it,
int missing_ok = flags & WRITE_TREE_MISSING_OK;
int dryrun = flags & WRITE_TREE_DRY_RUN;
int i;
+ int to_invalidate = 0;
if (0 <= it->entry_count && has_sha1_file(it->sha1))
return it->entry_count;
@@ -324,7 +325,14 @@ static int update_one(struct cache_tree *it,
if (!sub)
die("cache-tree.c: '%.*s' in '%s' not found",
entlen, path + baselen, path);
- i += sub->cache_tree->entry_count - 1;
+ i--; /* this entry is already counted in "sub" */
+ if (sub->cache_tree->entry_count < 0) {
+ i -= sub->cache_tree->entry_count;
+ sub->cache_tree->entry_count = -1;
+ to_invalidate = 1;
+ }
+ else
+ i += sub->cache_tree->entry_count;
sha1 = sub->cache_tree->sha1;
mode = S_IFDIR;
}
@@ -339,8 +347,23 @@ static int update_one(struct cache_tree *it,
mode, sha1_to_hex(sha1), entlen+baselen, path);
}
- if (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD))
- continue; /* entry being removed or placeholder */
+ /*
+ * CE_REMOVE entries are removed before the index is
+ * written to disk. Skip them to remain consistent
+ * with the future on-disk index.
+ */
+ if (ce->ce_flags & CE_REMOVE)
+ continue;
+
+ /*
+ * CE_INTENT_TO_ADD entries exist on on-disk index but
+ * they are not part of generated trees. Invalidate up
+ * to root to force cache-tree users to read elsewhere.
+ */
+ if (ce->ce_flags & CE_INTENT_TO_ADD) {
+ to_invalidate = 1;
+ continue;
+ }
strbuf_grow(&buffer, entlen + 100);
strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0');
@@ -360,7 +383,7 @@ static int update_one(struct cache_tree *it,
}
strbuf_release(&buffer);
- it->entry_count = i;
+ it->entry_count = to_invalidate ? -i : i;
#if DEBUG
fprintf(stderr, "cache-tree update-one (%d ent, %d subtree) %s\n",
it->entry_count, it->subtree_nr,
@@ -381,6 +404,15 @@ int cache_tree_update(struct cache_tree *it,
i = update_one(it, cache, entries, "", 0, flags);
if (i < 0)
return i;
+ /*
+ * update_one() uses negative entry_count as a way to mark an
+ * entry invalid _and_ pass the number of entries back to
+ * itself at the parent level. This is for internal use and
+ * should not be leaked out after the top-level update_one
+ * exits.
+ */
+ if (it->entry_count < 0)
+ it->entry_count = -1;
return 0;
}
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index ec35409..2a4a749 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -62,5 +62,25 @@ test_expect_success 'can "commit -a" with an i-t-a entry' '
git commit -a -m all
'
+test_expect_success 'cache-tree invalidates i-t-a paths' '
+ git reset --hard &&
+ mkdir dir &&
+ : >dir/foo &&
+ git add dir/foo &&
+ git commit -m foo &&
+
+ : >dir/bar &&
+ git add -N dir/bar &&
+ git diff --cached --name-only >actual &&
+ echo dir/bar >expect &&
+ test_cmp expect actual &&
+
+ git write-tree >/dev/null &&
+
+ git diff --cached --name-only >actual &&
+ echo dir/bar >expect &&
+ test_cmp expect actual
+'
+
test_done
--
1.8.0.rc2.23.g1fb49df
^ permalink raw reply related
* [PATCH v2] index-format.txt: be more liberal on what can represent invalid cache tree
From: Nguyễn Thái Ngọc Duy @ 2012-12-13 1:14 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <7v4njr5eac.fsf@alter.siamese.dyndns.org>
We have been writing -1 as "invalid" since day 1. On that same day we
accept all negative entry counts as "invalid". So in theory all C Git
versions out there would be happy to accept any negative numbers. JGit
seems to do exactly the same.
Correct the document to reflect the fact that -1 is not the only magic
number. At least one implementation, libgit2, is found to treat -1
this way.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
On Thu, Dec 13, 2012 at 1:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> We have been writing -1 as "invalid" since day 1. On that same day we
>> accept all negative entry counts as "invalid". So in theory all C Git
>> versions out there would be happy to accept any negative numbers. JGit
>> seems to do exactly the same.
>
> I am of two minds here.
>
> The existing code is being more lenient than specified when they
> read stuff others wrote, but it still adheres to -1 when writing.
> Allowing random implementations to write random negative values will
> close the door for us to later update the specification to encode
> more informatin about these invalid entries by using negative value
> other than -1 here.
How would that work with existing versions? If you write -2 in
cache-tree, the next time 1.8.0 updates cache tree it writes -1 back.
That loses whatever information you attach to -2. A new cache-tree
extension is probably better.
> I am OK with a reword to say "negative means invalid, and writers
> should write -1 for invalid entries", but without the latter half,
> this change is not justified.
Done.
Documentation/technical/index-format.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/technical/index-format.txt b/Documentation/technical/index-format.txt
index 9d25b30..ce28a7a 100644
--- a/Documentation/technical/index-format.txt
+++ b/Documentation/technical/index-format.txt
@@ -161,8 +161,9 @@ GIT index format
this span of index as a tree.
An entry can be in an invalidated state and is represented by having
- -1 in the entry_count field. In this case, there is no object name
- and the next entry starts immediately after the newline.
+ a negative number in the entry_count field. In this case, there is no
+ object name and the next entry starts immediately after the newline.
+ When writing an invalid entry, -1 should always be used as entry_count.
The entries are written out in the top-down, depth-first order. The
first entry represents the root level of the repository, followed by the
--
1.8.0.rc2.23.g1fb49df
^ permalink raw reply related
* Re: [PATCH v2] submodule: add 'deinit' command
From: W. Trevor King @ 2012-12-13 0:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jens Lehmann, Michael J Gruber, Phil Hord, Git, Heiko Voigt,
Jeff King, Shawn Pearce, Nahor
In-Reply-To: <7vsj7a268w.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 853 bytes --]
On Wed, Dec 12, 2012 at 03:35:59PM -0800, Junio C Hamano wrote:
> "W. Trevor King" <wking@tremily.us> writes:
>
> > Should `deinit` remove the submodule checkout, replace it with the
> > original gitlink, and clear the .git/config information then? That
> > would restore the user to the state they'd be in if they were never
> > interested in the submodule.
>
> AFAIU, "restore the user to the state" is the goal. I am not sure
> what you meant by "replace it with the original gitlink", though. A
> checkout with a submodule that the user is not interested in would
> have an empty directory at that path, no?
Ah yes, the gitlink is only in the index. Sorry for the noise.
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: directory permissions on AFS
From: Junio C Hamano @ 2012-12-13 0:17 UTC (permalink / raw)
To: Jaime Frey; +Cc: git
In-Reply-To: <0A6FA42F-986D-4C3C-BC50-9A7C0494C703@cs.wisc.edu>
Jaime Frey <jfrey@cs.wisc.edu> writes:
> Stracing git revealed that it successfully recreated the ./objects/fb
> and then failed to chmod() it. It failed because it tried to set the
> S_ISGID bit, which mere mortals cannot do on AFS. Manually recreating
> all of these directories solves the problem.
We fix directory permissions after creating any directory under .git
with the same code, so that in a repository shared by group, new
subdirectories created by a random somebody who belongs to that
group will belong to that group (we also chmod to g+wx in case such
a random somebody has overly strict umask). Instead of running
chown(2) on every new file created by us, we let the filesystem to
take care of it by telling the directories we create that new files
in them should inherit their group ownership.
What we were worried about back when we decided to use S_ISGID was a
scenario like this:
* A repository is shared by group "src".
* A user belongs to the group "src". That group may or may not be
his primary group (i.e. "mkdir foo" done at random place by him
may not belong to the "src" group).
* The user attempts to create a new branch "foo/bar" by pushing
from outside. There is no other branch whose name is
"foo/anything" when this happens.
* An equivalent of "mkdir -p .git/refs/heads/foo" needs to be done
before an equivalent of "echo $sha >.git/refs/heads/foo/bar"
happens to accept this push. We want "foo" and "bar" to belong
to "src" group and they get appropriate permission bits suitable
to be accessed by the members of the "src" group.
The story is the same for loose objects and their fan-out directory.
Storing a commit object fb/012345... may need to create the leading
fan-out ".git/objects/fb" and we want that directory and any future
files created in it to belong to the "src" group.
Any alternative implementation that achieves the same result that
works on AFS can be substituted with the current code, or made
conditionally activated on AFS.
^ permalink raw reply
* Re: Fwd: possible Improving diff algoritm
From: Michael Haggerty @ 2012-12-13 0:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Morten Welinder, Kevin, git
In-Reply-To: <7vpq2f2az4.fsf@alter.siamese.dyndns.org>
On 12/12/2012 10:53 PM, Junio C Hamano wrote:
> Morten Welinder <mwelinder@gmail.com> writes:
>
>> Is there a reason why picking among the choices in a sliding window
>> must be contents neutral?
>
> Sorry, you might be getting at something interesting but I do not
> understand the question. I have no idea what you mean by "contents
> neutral".
>
> Picking between these two choices
>
> /** + /**
> + * Default parent + * Default parent
> + * + *
> + * @var int + * @var int
> + * @access protected + * @access protected
> + * @index + * @index
> + */ + */
> + protected $defaultParent; + protected $defaultParent;
> + +
> + /** /**
>
> would not affect the correctness of the patch. You may pick
> whatever you deem the most desirable, but your answer must be a
> correct patch (the definition of "correct" here is "applying that
> patch to the preimage produces the intended postimage").
>
> And I think if you inserted a block of text B after a context C
> where the tail of B matches the tail of C like the above, you can
> shift what you treat as "inserted" up and still come up with a
> correct patch.
I have the feeling that a few crude heuristics would go a long way
towards improving diffs like this. For example:
* Prefer to have an add/remove block that has balanced begin/end pairs
(where begin/end pairs might be opening and closing parentheses,
brackets, braces, and angle brackets, "/*" and "*/", and perhaps a
couple of other things. For SGML-like text begin and end tags could be
matched up.
It would be possible to read these begin/end pairs from a
filetype-specific table or configuration setting, though this would add
complication and would also make it possible that diffs generated by two
different people are not identical if their configurations differ.
* Prefer to have a block where the first non-blank line of the block and
the first non-blank line after the block are indented by the same amount.
* Prefer to have a block with trailing (as opposed to leading or
embedded) blank lines--the more the better.
The beautiful thing is that even if the heuristics sometimes fail, the
correctness of the patch (in the sense that you have defined) is not
compromised.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ 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