* Re: [PATCH v5 07/24] files-backend: add and use files_refname_path()
From: Michael Haggerty @ 2017-03-09 12:24 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-8-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> Keep repo-related path handling in one place. This will make it easier
> to add submodule/multiworktree support later.
>
> This automatically adds the "if submodule then use the submodule version
> of git_path" to other call sites too. But it does not mean those
> operations are sumodule-ready. Not yet.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs/files-backend.c | 45 +++++++++++++++++++++++++--------------------
> 1 file changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 7b4ea4c56..72f4e1746 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> [...]
> @@ -1251,10 +1263,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
> size_t path_baselen;
> int err = 0;
>
> - if (refs->submodule)
> - err = strbuf_git_path_submodule(&path, refs->submodule, "%s", dirname);
> - else
> - strbuf_git_path(&path, "%s", dirname);
> + files_refname_path(refs, &path, dirname);
> path_baselen = path.len;
>
> if (err) {
I just noticed another thing. After this change, `err` is never set, so
the `if (err)` block (and `err` itself) can be deleted.
> [...]
Michael
^ permalink raw reply
* Re: [PATCH v2] t2027: avoid using pipes
From: Christian Couder @ 2017-03-09 12:30 UTC (permalink / raw)
To: Prathamesh Chavan; +Cc: git
In-Reply-To: <0102015ab27c6633-c61f56f2-0504-4af3-badc-34246cf635aa-000000@eu-west-1.amazonses.com>
On Thu, Mar 9, 2017 at 10:53 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> Whenever a git command is present in the upstream of a pipe, its failure
> gets masked by piping and hence it should be avoided for testing the
> upstream git command. By writing out the output of the git command to
> a file, we can test the exit codes of both the commands as a failure exit
> code in any command is able to stop the && chain.
>
> Signed-off-by: Prathamesh <pc44800@gmail.com>
> ---
Please add in Cc those who previously commented on the patch.
> t/t2027-worktree-list.sh | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> index 848da5f..daa7a04 100755
> --- a/t/t2027-worktree-list.sh
> +++ b/t/t2027-worktree-list.sh
> @@ -31,7 +31,7 @@ test_expect_success '"list" all worktrees from main' '
> test_when_finished "rm -rf here && git worktree prune" &&
> git worktree add --detach here master &&
> echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
> - git worktree list | sed "s/ */ /g" >actual &&
> + git worktree list >out && sed "s/ */ /g" <out >actual &&
I still think that it would be better if the 'sed' commend was on its
own line like this:
+ git worktree list >out &&
+ sed "s/ */ /g" <out >actual &&
> test_cmp expect actual
> '
^ permalink raw reply
* Re: [PATCH] branch & tag: Add a --no-contains option
From: Jeff King @ 2017-03-09 12:51 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano, Lars Hjemli, Christian Couder
In-Reply-To: <CACBZZX5i+8bQLhLB4DnUAaUw1vA_KQjhtNBvm7drLepPAFFbAQ@mail.gmail.com>
On Thu, Mar 09, 2017 at 01:12:08PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > I'm almost certain this is because the contains_tag_algo one doesn't
> > clean up the flag bits it sets on the commit objects. So running it
> > twice in the same process is going to give you nonsense results.
>
> Yeah indeed.
>
> I tried to hack something up to avoid this, but the
> lookup_commit_reference_gently() we call will return the same
> object.parent pointer for two invocations, i.e. the underlying
> {commit,object}.c API has a cache of objects it returns, couldn't find
> some way to quickly make it burst that cache.
Yeah, you'll always get the same struct for a given sha1.
> The other approach of making contains_tag_algo() itself detect that
> it's been called before (or us passing a flag) and going around
> setting commit.object.flags on everything to 0 seemed even more
> brittle, particularly since I think between filter->with_commit &
> filter->no_commit we might end up visiting different commits, so it's
> not easy to just clear it.
You can clear the marks with clear_object_flags(). But I don't think
that type of solution will quite help us here. We consider each ref
independently and ask "does it match --contains" and "does it match
--no-contains?". So there is no moment where we are done with all of the
--contains marks, and can move on to the --no-contains ones. The lookups
are interleaved.
We could move to doing them in chunks (the way filter->merge_commit
works), and then clearing in between. Or we could use a separate bitset.
The patch below does that.
> I'm happy to hack on it given some pointers, will visit it again, but
> for now unless I'm missing something obvious / you can point out some
> way to hack this up I'll just submit v2 with the combination of
> --contains & --no-contains dying with a TODO message.
>
> The patch without that functionality is still really useful, and we
> can implement that later.
I'm not opposed to that, though see what you think of the patch below.
It's a bit noisy but it's conceptually pretty straightforward. It should
hopefully be obvious how you'd add in a separate contains_cache for the
"without" case.
Looking at this, I'm pretty sure that using "--contains" with "--merged"
has similar problems, as they both use the UNINTERESTING bit. So even
without your patch, there is a lurking bug.
diff --git a/ref-filter.c b/ref-filter.c
index 3820b21cc..42b1bc463 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -14,6 +14,7 @@
#include "git-compat-util.h"
#include "version.h"
#include "trailer.h"
+#include "commit-slab.h"
typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
@@ -1137,10 +1138,22 @@ static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom
*v = &ref->value[atom];
}
+/*
+ * Unknown has to be "0" here, because that's what unfilled entries in our slab
+ * will return.
+ */
enum contains_result {
- CONTAINS_UNKNOWN = -1,
- CONTAINS_NO = 0,
- CONTAINS_YES = 1
+ CONTAINS_UNKNOWN = 0,
+ CONTAINS_NO = 1,
+ CONTAINS_YES = 2,
+};
+
+define_commit_slab(contains_cache, enum contains_result);
+
+struct ref_filter_cbdata {
+ struct ref_array *array;
+ struct ref_filter *filter;
+ struct contains_cache contains_cache;
};
/*
@@ -1171,24 +1184,25 @@ static int in_commit_list(const struct commit_list *want, struct commit *c)
* Do not recurse to find out, though, but return -1 if inconclusive.
*/
static enum contains_result contains_test(struct commit *candidate,
- const struct commit_list *want)
+ const struct commit_list *want,
+ struct contains_cache *cache)
{
- /* was it previously marked as containing a want commit? */
- if (candidate->object.flags & TMP_MARK)
- return 1;
- /* or marked as not possibly containing a want commit? */
- if (candidate->object.flags & UNINTERESTING)
- return 0;
+ enum contains_result *cached = contains_cache_at(cache, candidate);
+
+ /* if we already found the answer, return it without traversing */
+ if (*cached)
+ return *cached;
+
/* or are we it? */
if (in_commit_list(want, candidate)) {
- candidate->object.flags |= TMP_MARK;
- return 1;
+ *cached = CONTAINS_YES;
+ return *cached;
}
if (parse_commit(candidate) < 0)
- return 0;
+ return CONTAINS_NO;
- return -1;
+ return CONTAINS_UNKNOWN;
}
static void push_to_contains_stack(struct commit *candidate, struct contains_stack *contains_stack)
@@ -1199,10 +1213,11 @@ static void push_to_contains_stack(struct commit *candidate, struct contains_sta
}
static enum contains_result contains_tag_algo(struct commit *candidate,
- const struct commit_list *want)
+ const struct commit_list *want,
+ struct contains_cache *cache)
{
struct contains_stack contains_stack = { 0, 0, NULL };
- int result = contains_test(candidate, want);
+ enum contains_result result = contains_test(candidate, want, cache);
if (result != CONTAINS_UNKNOWN)
return result;
@@ -1214,16 +1229,16 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
struct commit_list *parents = entry->parents;
if (!parents) {
- commit->object.flags |= UNINTERESTING;
+ *contains_cache_at(cache, commit) = CONTAINS_NO;
contains_stack.nr--;
}
/*
* If we just popped the stack, parents->item has been marked,
- * therefore contains_test will return a meaningful 0 or 1.
+ * therefore contains_test will return a meaningful yes/no.
*/
- else switch (contains_test(parents->item, want)) {
+ else switch (contains_test(parents->item, want, cache)) {
case CONTAINS_YES:
- commit->object.flags |= TMP_MARK;
+ *contains_cache_at(cache, commit) = CONTAINS_YES;
contains_stack.nr--;
break;
case CONTAINS_NO:
@@ -1235,13 +1250,14 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
}
}
free(contains_stack.contains_stack);
- return contains_test(candidate, want);
+ return contains_test(candidate, want, cache);
}
-static int commit_contains(struct ref_filter *filter, struct commit *commit)
+static int commit_contains(struct ref_filter *filter, struct commit *commit,
+ struct contains_cache *cache)
{
if (filter->with_commit_tag_algo)
- return contains_tag_algo(commit, filter->with_commit);
+ return contains_tag_algo(commit, filter->with_commit, cache) == CONTAINS_YES;
return is_descendant_of(commit, filter->with_commit);
}
@@ -1438,7 +1454,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
return 0;
/* We perform the filtering for the '--contains' option */
if (filter->with_commit &&
- !commit_contains(filter, commit))
+ !commit_contains(filter, commit, &ref_cbdata->contains_cache))
return 0;
}
@@ -1538,6 +1554,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
broken = 1;
filter->kind = type & FILTER_REFS_KIND_MASK;
+ init_contains_cache(&ref_cbdata.contains_cache);
+
/* Simple per-ref filtering */
if (!filter->kind)
die("filter_refs: invalid type");
@@ -1560,6 +1578,7 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
head_ref(ref_filter_handler, &ref_cbdata);
}
+ clear_contains_cache(&ref_cbdata.contains_cache);
/* Filters that need revision walking */
if (filter->merge_commit)
diff --git a/ref-filter.h b/ref-filter.h
index 7b05592ba..89af9f451 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -71,11 +71,6 @@ struct ref_filter {
verbose;
};
-struct ref_filter_cbdata {
- struct ref_array *array;
- struct ref_filter *filter;
-};
-
/* Macros for checking --merged and --no-merged options */
#define _OPT_MERGED_NO_MERGED(option, filter, h) \
{ OPTION_CALLBACK, 0, option, (filter), N_("commit"), (h), \
^ permalink raw reply related
* Re: [PATCH] branch: honor --abbrev/--no-abbrev in --list mode
From: Jakub Narębski @ 2017-03-09 13:25 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Karthik Nayak, Guillaume Wenzek
In-Reply-To: <xmqqvarj1kix.fsf_-_@gitster.mtv.corp.google.com>
W dniu 08.03.2017 o 23:16, Junio C Hamano pisze:
> diff --git a/builtin/branch.c b/builtin/branch.c
> index cbaa6d03c0..537c47811a 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -335,9 +335,18 @@ static char *build_format(struct ref_filter *filter, int maxwidth, const char *r
> branch_get_color(BRANCH_COLOR_CURRENT));
>
> if (filter->verbose) {
> + struct strbuf obname = STRBUF_INIT;
> +
> + if (filter->abbrev < 0)
> + strbuf_addf(&obname, "%%(objectname:short)");
> + else if (!filter->abbrev)
> + strbuf_addf(&obname, "%%(objectname)");
> + else
> + strbuf_addf(&obname, " %%(objectname:short=%d) ", filter->abbrev);
^ ^
I wonder why the last one has leading space --/ and trailing one -----/
The rest (for default abbrev and for no abbrev do not).
--
Jakub Narębski
^ permalink raw reply
* Re: [GSOC] Regarding microprojects for gsoc 2017
From: Christian Couder @ 2017-03-09 13:25 UTC (permalink / raw)
To: Avinash Tiwary; +Cc: git
In-Reply-To: <CAAdPw+d1GR3xZcoQ6jtd8EXhQKM2GRSmeb07iSExDiugXoX0Pg@mail.gmail.com>
On Thu, Mar 9, 2017 at 2:14 PM, Avinash Tiwary <tiwarysriavi@gmail.com> wrote:
> [GSOC]
> Sir, i am interested in git projects and i want to complete one of the git
> microprojects. But i am unable to figure out which git repo they are talking
> about. Since , there is no link provided. Please help
On the microproject page there is:
"Download the source code: clone the repository using the Git via Git
instructions and read the README file."
where "Git via Git" is a link to https://git-scm.com/downloads where there is:
================
Git via Git
If you already have Git installed, you can get the latest development
version via Git itself:
git clone https://github.com/git/git
You can also always browse the current contents of the git repository
using the web interface.
================
and where "web interface" is a link to https://github.com/git/git
^ permalink raw reply
* [PATCH 0/4] fix object flag pollution in "tag --contains"
From: Jeff King @ 2017-03-09 13:27 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano, Lars Hjemli, Christian Couder
In-Reply-To: <20170309125132.tubwxtneffok4nrc@sigill.intra.peff.net>
On Thu, Mar 09, 2017 at 07:51:32AM -0500, Jeff King wrote:
> Looking at this, I'm pretty sure that using "--contains" with "--merged"
> has similar problems, as they both use the UNINTERESTING bit. So even
> without your patch, there is a lurking bug.
I wasn't able to come up with a simple case that actually demonstrates
the bug. But I feel like it has to be triggerable with the right
sequence of history.
Even without that, though, I feel like moving away from this flag usage
is a good cleanup. Here's a cleaned-up series. What do you think of
building your patch on top?
We can do it the other way around if you prefer.
[1/4]: ref-filter: move ref_cbdata definition into ref-filter.c
[2/4]: ref-filter: use contains_result enum consistently
[3/4]: ref-filter: die on parse_commit errors
[4/4]: ref-filter: use separate cache for contains_tag_algo
ref-filter.c | 70 ++++++++++++++++++++++++++++++++++++++----------------------
ref-filter.h | 5 -----
2 files changed, 44 insertions(+), 31 deletions(-)
-Peff
^ permalink raw reply
* [PATCH 1/4] ref-filter: move ref_cbdata definition into ref-filter.c
From: Jeff King @ 2017-03-09 13:27 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano, Lars Hjemli, Christian Couder
In-Reply-To: <20170309132728.c57ltzel746l366a@sigill.intra.peff.net>
This is an implementation detail of how filter_refs() works,
and does not need to be exposed to the outside world. This
will become more important in future patches as we add new
private data types to it.
Signed-off-by: Jeff King <peff@peff.net>
---
ref-filter.c | 5 +++++
ref-filter.h | 5 -----
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 1ec0fb839..6546dba73 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1476,6 +1476,11 @@ enum contains_result {
CONTAINS_YES = 1
};
+struct ref_filter_cbdata {
+ struct ref_array *array;
+ struct ref_filter *filter;
+};
+
/*
* Mimicking the real stack, this stack lives on the heap, avoiding stack
* overflows.
diff --git a/ref-filter.h b/ref-filter.h
index 154e24c40..e738c5dfd 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -71,11 +71,6 @@ struct ref_filter {
verbose;
};
-struct ref_filter_cbdata {
- struct ref_array *array;
- struct ref_filter *filter;
-};
-
/* Macros for checking --merged and --no-merged options */
#define _OPT_MERGED_NO_MERGED(option, filter, h) \
{ OPTION_CALLBACK, 0, option, (filter), N_("commit"), (h), \
--
2.12.0.445.g818af77e0
^ permalink raw reply related
* [PATCH 2/4] ref-filter: use contains_result enum consistently
From: Jeff King @ 2017-03-09 13:28 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano, Lars Hjemli, Christian Couder
In-Reply-To: <20170309132728.c57ltzel746l366a@sigill.intra.peff.net>
Commit cbc60b672 (git tag --contains: avoid stack overflow,
2014-04-24) adapted the -1/0/1 contains status into a
tri-state enum. However, some of the code still used the
numeric values, or assumed that no/yes correspond to C's
boolean true/false.
Let's switch to using the symbolic values everywhere, which
will make it easier to change them.
Signed-off-by: Jeff King <peff@peff.net>
---
ref-filter.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 6546dba73..631978a4f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1513,20 +1513,20 @@ static enum contains_result contains_test(struct commit *candidate,
{
/* was it previously marked as containing a want commit? */
if (candidate->object.flags & TMP_MARK)
- return 1;
+ return CONTAINS_YES;
/* or marked as not possibly containing a want commit? */
if (candidate->object.flags & UNINTERESTING)
- return 0;
+ return CONTAINS_NO;
/* or are we it? */
if (in_commit_list(want, candidate)) {
candidate->object.flags |= TMP_MARK;
- return 1;
+ return CONTAINS_YES;
}
if (parse_commit(candidate) < 0)
- return 0;
+ return CONTAINS_NO;
- return -1;
+ return CONTAINS_UNKNOWN;
}
static void push_to_contains_stack(struct commit *candidate, struct contains_stack *contains_stack)
@@ -1540,7 +1540,7 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
const struct commit_list *want)
{
struct contains_stack contains_stack = { 0, 0, NULL };
- int result = contains_test(candidate, want);
+ enum contains_result result = contains_test(candidate, want);
if (result != CONTAINS_UNKNOWN)
return result;
@@ -1557,7 +1557,7 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
}
/*
* If we just popped the stack, parents->item has been marked,
- * therefore contains_test will return a meaningful 0 or 1.
+ * therefore contains_test will return a meaningful yes/no.
*/
else switch (contains_test(parents->item, want)) {
case CONTAINS_YES:
@@ -1579,7 +1579,7 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
static int commit_contains(struct ref_filter *filter, struct commit *commit)
{
if (filter->with_commit_tag_algo)
- return contains_tag_algo(commit, filter->with_commit);
+ return contains_tag_algo(commit, filter->with_commit) == CONTAINS_YES;
return is_descendant_of(commit, filter->with_commit);
}
--
2.12.0.445.g818af77e0
^ permalink raw reply related
* [PATCH 3/4] ref-filter: die on parse_commit errors
From: Jeff King @ 2017-03-09 13:29 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano, Lars Hjemli, Christian Couder
In-Reply-To: <20170309132728.c57ltzel746l366a@sigill.intra.peff.net>
The tag-contains algorithm quietly returns "does not
contain" when parse_commit() fails. But a parse failure is
an indication that the repository is corrupt. We should die
loudly rather than producing a bogus result.
Signed-off-by: Jeff King <peff@peff.net>
---
ref-filter.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 631978a4f..5cb49b7c2 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1523,9 +1523,7 @@ static enum contains_result contains_test(struct commit *candidate,
return CONTAINS_YES;
}
- if (parse_commit(candidate) < 0)
- return CONTAINS_NO;
-
+ parse_commit_or_die(candidate);
return CONTAINS_UNKNOWN;
}
--
2.12.0.445.g818af77e0
^ permalink raw reply related
* [PATCH 4/4] ref-filter: use separate cache for contains_tag_algo
From: Jeff King @ 2017-03-09 13:29 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano, Lars Hjemli, Christian Couder
In-Reply-To: <20170309132728.c57ltzel746l366a@sigill.intra.peff.net>
The algorithm which powers "tag --contains" uses the
TMP_MARK and UNINTERESTING bits, but never cleans up after
itself. As a result, stale UNINTERESTING bits may impact
later traversals (like "--merged").
We could fix this by clearing the bits after we're done with
the --contains traversal. That would be enough to fix the
existing problem, but it leaves future developers in a bad
spot: they cannot add other traversals that operate
simultaneously with --contains (e.g., if you wanted to add
"--no-contains" and use both filters at the same time).
Instead, we can use a commit slab to store our cached
results, which will store the bits outside of the commit
structs entirely. This adds an extra level of indirection,
but in my tests (running "git tag --contains HEAD" on
linux.git), there was no measurable slowdown.
Signed-off-by: Jeff King <peff@peff.net>
---
ref-filter.c | 55 +++++++++++++++++++++++++++++++++++--------------------
1 file changed, 35 insertions(+), 20 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 5cb49b7c2..7eeecc608 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -15,6 +15,7 @@
#include "version.h"
#include "trailer.h"
#include "wt-status.h"
+#include "commit-slab.h"
static struct ref_msg {
const char *gone;
@@ -1470,15 +1471,22 @@ static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom
*v = &ref->value[atom];
}
+/*
+ * Unknown has to be "0" here, because that's the default value for
+ * contains_cache slab entries that have not yet been assigned.
+ */
enum contains_result {
- CONTAINS_UNKNOWN = -1,
- CONTAINS_NO = 0,
- CONTAINS_YES = 1
+ CONTAINS_UNKNOWN = 0,
+ CONTAINS_NO,
+ CONTAINS_YES
};
+define_commit_slab(contains_cache, enum contains_result);
+
struct ref_filter_cbdata {
struct ref_array *array;
struct ref_filter *filter;
+ struct contains_cache contains_cache;
};
/*
@@ -1509,20 +1517,22 @@ static int in_commit_list(const struct commit_list *want, struct commit *c)
* Do not recurse to find out, though, but return -1 if inconclusive.
*/
static enum contains_result contains_test(struct commit *candidate,
- const struct commit_list *want)
+ const struct commit_list *want,
+ struct contains_cache *cache)
{
- /* was it previously marked as containing a want commit? */
- if (candidate->object.flags & TMP_MARK)
- return CONTAINS_YES;
- /* or marked as not possibly containing a want commit? */
- if (candidate->object.flags & UNINTERESTING)
- return CONTAINS_NO;
+ enum contains_result *cached = contains_cache_at(cache, candidate);
+
+ /* If we already have the answer cached, return that. */
+ if (*cached)
+ return *cached;
+
/* or are we it? */
if (in_commit_list(want, candidate)) {
- candidate->object.flags |= TMP_MARK;
+ *cached = CONTAINS_YES;
return CONTAINS_YES;
}
+ /* Otherwise, we don't know; prepare to recurse */
parse_commit_or_die(candidate);
return CONTAINS_UNKNOWN;
}
@@ -1535,10 +1545,11 @@ static void push_to_contains_stack(struct commit *candidate, struct contains_sta
}
static enum contains_result contains_tag_algo(struct commit *candidate,
- const struct commit_list *want)
+ const struct commit_list *want,
+ struct contains_cache *cache)
{
struct contains_stack contains_stack = { 0, 0, NULL };
- enum contains_result result = contains_test(candidate, want);
+ enum contains_result result = contains_test(candidate, want, cache);
if (result != CONTAINS_UNKNOWN)
return result;
@@ -1550,16 +1561,16 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
struct commit_list *parents = entry->parents;
if (!parents) {
- commit->object.flags |= UNINTERESTING;
+ *contains_cache_at(cache, commit) = CONTAINS_NO;
contains_stack.nr--;
}
/*
* If we just popped the stack, parents->item has been marked,
* therefore contains_test will return a meaningful yes/no.
*/
- else switch (contains_test(parents->item, want)) {
+ else switch (contains_test(parents->item, want, cache)) {
case CONTAINS_YES:
- commit->object.flags |= TMP_MARK;
+ *contains_cache_at(cache, commit) = CONTAINS_YES;
contains_stack.nr--;
break;
case CONTAINS_NO:
@@ -1571,13 +1582,14 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
}
}
free(contains_stack.contains_stack);
- return contains_test(candidate, want);
+ return contains_test(candidate, want, cache);
}
-static int commit_contains(struct ref_filter *filter, struct commit *commit)
+static int commit_contains(struct ref_filter *filter, struct commit *commit,
+ struct contains_cache *cache)
{
if (filter->with_commit_tag_algo)
- return contains_tag_algo(commit, filter->with_commit) == CONTAINS_YES;
+ return contains_tag_algo(commit, filter->with_commit, cache) == CONTAINS_YES;
return is_descendant_of(commit, filter->with_commit);
}
@@ -1774,7 +1786,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
return 0;
/* We perform the filtering for the '--contains' option */
if (filter->with_commit &&
- !commit_contains(filter, commit))
+ !commit_contains(filter, commit, &ref_cbdata->contains_cache))
return 0;
}
@@ -1874,6 +1886,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
broken = 1;
filter->kind = type & FILTER_REFS_KIND_MASK;
+ init_contains_cache(&ref_cbdata.contains_cache);
+
/* Simple per-ref filtering */
if (!filter->kind)
die("filter_refs: invalid type");
@@ -1896,6 +1910,7 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
head_ref(ref_filter_handler, &ref_cbdata);
}
+ clear_contains_cache(&ref_cbdata.contains_cache);
/* Filters that need revision walking */
if (filter->merge_commit)
--
2.12.0.445.g818af77e0
^ permalink raw reply related
* Re: [PATCH] branch & tag: Add a --no-contains option
From: Ævar Arnfjörð Bjarmason @ 2017-03-09 14:52 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, Lars Hjemli, Christian Couder
In-Reply-To: <20170309125132.tubwxtneffok4nrc@sigill.intra.peff.net>
On Thu, Mar 9, 2017 at 1:51 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 09, 2017 at 01:12:08PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > I'm almost certain this is because the contains_tag_algo one doesn't
>> > clean up the flag bits it sets on the commit objects. So running it
>> > twice in the same process is going to give you nonsense results.
>>
>> Yeah indeed.
>>
>> I tried to hack something up to avoid this, but the
>> lookup_commit_reference_gently() we call will return the same
>> object.parent pointer for two invocations, i.e. the underlying
>> {commit,object}.c API has a cache of objects it returns, couldn't find
>> some way to quickly make it burst that cache.
>
> Yeah, you'll always get the same struct for a given sha1.
>
>> The other approach of making contains_tag_algo() itself detect that
>> it's been called before (or us passing a flag) and going around
>> setting commit.object.flags on everything to 0 seemed even more
>> brittle, particularly since I think between filter->with_commit &
>> filter->no_commit we might end up visiting different commits, so it's
>> not easy to just clear it.
>
> You can clear the marks with clear_object_flags(). But I don't think
> that type of solution will quite help us here. We consider each ref
> independently and ask "does it match --contains" and "does it match
> --no-contains?". So there is no moment where we are done with all of the
> --contains marks, and can move on to the --no-contains ones. The lookups
> are interleaved.
>
> We could move to doing them in chunks (the way filter->merge_commit
> works), and then clearing in between. Or we could use a separate bitset.
> The patch below does that.
>
>> I'm happy to hack on it given some pointers, will visit it again, but
>> for now unless I'm missing something obvious / you can point out some
>> way to hack this up I'll just submit v2 with the combination of
>> --contains & --no-contains dying with a TODO message.
>>
>> The patch without that functionality is still really useful, and we
>> can implement that later.
>
> I'm not opposed to that, though see what you think of the patch below.
> It's a bit noisy but it's conceptually pretty straightforward. It should
> hopefully be obvious how you'd add in a separate contains_cache for the
> "without" case.
I wonder how worthwhile making the combination of options case fast &
adding more complexity is, as opposed to just using the slower path
for those cases via:
diff --git a/builtin/tag.c b/builtin/tag.c
index 737a83028a..d90e8675a8 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -53,7 +53,10 @@ static int list_tags(struct ref_filter *filter,
struct ref_sorting *sorting, con
}
verify_ref_format(format);
- filter->with_commit_tag_algo = 1;
+ if ((filter->merge_commit + filter->with_commit +
filter->no_commit) > 1)
+ filter->with_commit_tag_algo = 0;
+ else
+ filter->with_commit_tag_algo = 1;
filter_refs(&array, filter, FILTER_REFS_TAGS);
ref_array_sort(sorting, &array);
I think I'll amend the tip of my WIP v2 to have something like that,
and then we can follow-up with making these cases where you supply
multiple options faster.
> Looking at this, I'm pretty sure that using "--contains" with "--merged"
> has similar problems, as they both use the UNINTERESTING bit. So even
> without your patch, there is a lurking bug.
I'm currently running this:
https://gist.github.com/avar/45cf288ce7cdc43e7395c6cbf9a98d68
with this patch to builtin/tag.c:
diff --git a/builtin/tag.c b/builtin/tag.c
index 737a83028a..b93c5e1754 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -56,2 +56,4 @@ static int list_tags(struct ref_filter *filter,
struct ref_sorting *sorting, con
filter->with_commit_tag_algo = 1;
+ if (getenv("GIT_NO_TAG_ALGO"))
+ filter->with_commit_tag_algo = 0;
To smoke out any combinations of commits in git.git where with &
without the tag algo we already return different results, nothing so
far, but it's slow going.
> diff --git a/ref-filter.c b/ref-filter.c
> index 3820b21cc..42b1bc463 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -14,6 +14,7 @@
> #include "git-compat-util.h"
> #include "version.h"
> #include "trailer.h"
> +#include "commit-slab.h"
>
> typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>
> @@ -1137,10 +1138,22 @@ static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom
> *v = &ref->value[atom];
> }
>
> +/*
> + * Unknown has to be "0" here, because that's what unfilled entries in our slab
> + * will return.
> + */
> enum contains_result {
> - CONTAINS_UNKNOWN = -1,
> - CONTAINS_NO = 0,
> - CONTAINS_YES = 1
> + CONTAINS_UNKNOWN = 0,
> + CONTAINS_NO = 1,
> + CONTAINS_YES = 2,
> +};
> +
> +define_commit_slab(contains_cache, enum contains_result);
> +
> +struct ref_filter_cbdata {
> + struct ref_array *array;
> + struct ref_filter *filter;
> + struct contains_cache contains_cache;
> };
>
> /*
> @@ -1171,24 +1184,25 @@ static int in_commit_list(const struct commit_list *want, struct commit *c)
> * Do not recurse to find out, though, but return -1 if inconclusive.
> */
> static enum contains_result contains_test(struct commit *candidate,
> - const struct commit_list *want)
> + const struct commit_list *want,
> + struct contains_cache *cache)
> {
> - /* was it previously marked as containing a want commit? */
> - if (candidate->object.flags & TMP_MARK)
> - return 1;
> - /* or marked as not possibly containing a want commit? */
> - if (candidate->object.flags & UNINTERESTING)
> - return 0;
> + enum contains_result *cached = contains_cache_at(cache, candidate);
> +
> + /* if we already found the answer, return it without traversing */
> + if (*cached)
> + return *cached;
> +
> /* or are we it? */
> if (in_commit_list(want, candidate)) {
> - candidate->object.flags |= TMP_MARK;
> - return 1;
> + *cached = CONTAINS_YES;
> + return *cached;
> }
>
> if (parse_commit(candidate) < 0)
> - return 0;
> + return CONTAINS_NO;
>
> - return -1;
> + return CONTAINS_UNKNOWN;
> }
>
> static void push_to_contains_stack(struct commit *candidate, struct contains_stack *contains_stack)
> @@ -1199,10 +1213,11 @@ static void push_to_contains_stack(struct commit *candidate, struct contains_sta
> }
>
> static enum contains_result contains_tag_algo(struct commit *candidate,
> - const struct commit_list *want)
> + const struct commit_list *want,
> + struct contains_cache *cache)
> {
> struct contains_stack contains_stack = { 0, 0, NULL };
> - int result = contains_test(candidate, want);
> + enum contains_result result = contains_test(candidate, want, cache);
>
> if (result != CONTAINS_UNKNOWN)
> return result;
> @@ -1214,16 +1229,16 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
> struct commit_list *parents = entry->parents;
>
> if (!parents) {
> - commit->object.flags |= UNINTERESTING;
> + *contains_cache_at(cache, commit) = CONTAINS_NO;
> contains_stack.nr--;
> }
> /*
> * If we just popped the stack, parents->item has been marked,
> - * therefore contains_test will return a meaningful 0 or 1.
> + * therefore contains_test will return a meaningful yes/no.
> */
> - else switch (contains_test(parents->item, want)) {
> + else switch (contains_test(parents->item, want, cache)) {
> case CONTAINS_YES:
> - commit->object.flags |= TMP_MARK;
> + *contains_cache_at(cache, commit) = CONTAINS_YES;
> contains_stack.nr--;
> break;
> case CONTAINS_NO:
> @@ -1235,13 +1250,14 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
> }
> }
> free(contains_stack.contains_stack);
> - return contains_test(candidate, want);
> + return contains_test(candidate, want, cache);
> }
>
> -static int commit_contains(struct ref_filter *filter, struct commit *commit)
> +static int commit_contains(struct ref_filter *filter, struct commit *commit,
> + struct contains_cache *cache)
> {
> if (filter->with_commit_tag_algo)
> - return contains_tag_algo(commit, filter->with_commit);
> + return contains_tag_algo(commit, filter->with_commit, cache) == CONTAINS_YES;
> return is_descendant_of(commit, filter->with_commit);
> }
>
> @@ -1438,7 +1454,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
> return 0;
> /* We perform the filtering for the '--contains' option */
> if (filter->with_commit &&
> - !commit_contains(filter, commit))
> + !commit_contains(filter, commit, &ref_cbdata->contains_cache))
> return 0;
> }
>
> @@ -1538,6 +1554,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
> broken = 1;
> filter->kind = type & FILTER_REFS_KIND_MASK;
>
> + init_contains_cache(&ref_cbdata.contains_cache);
> +
> /* Simple per-ref filtering */
> if (!filter->kind)
> die("filter_refs: invalid type");
> @@ -1560,6 +1578,7 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
> head_ref(ref_filter_handler, &ref_cbdata);
> }
>
> + clear_contains_cache(&ref_cbdata.contains_cache);
>
> /* Filters that need revision walking */
> if (filter->merge_commit)
> diff --git a/ref-filter.h b/ref-filter.h
> index 7b05592ba..89af9f451 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -71,11 +71,6 @@ struct ref_filter {
> verbose;
> };
>
> -struct ref_filter_cbdata {
> - struct ref_array *array;
> - struct ref_filter *filter;
> -};
> -
> /* Macros for checking --merged and --no-merged options */
> #define _OPT_MERGED_NO_MERGED(option, filter, h) \
> { OPTION_CALLBACK, 0, option, (filter), N_("commit"), (h), \
^ permalink raw reply related
* Re: [PATCH] branch & tag: Add a --no-contains option
From: Jeff King @ 2017-03-09 14:55 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano, Lars Hjemli, Christian Couder
In-Reply-To: <CACBZZX4W7brFe5ecTvQPMmf4X5_zH+dw3cB5xeVg+2hWYps0Ug@mail.gmail.com>
On Thu, Mar 09, 2017 at 03:52:09PM +0100, Ævar Arnfjörð Bjarmason wrote:
> - filter->with_commit_tag_algo = 1;
> + if ((filter->merge_commit + filter->with_commit +
> filter->no_commit) > 1)
> + filter->with_commit_tag_algo = 0;
> + else
> + filter->with_commit_tag_algo = 1;
> filter_refs(&array, filter, FILTER_REFS_TAGS);
> ref_array_sort(sorting, &array);
>
> I think I'll amend the tip of my WIP v2 to have something like that,
> and then we can follow-up with making these cases where you supply
> multiple options faster.
Yeah, that's another option. I think you may find that it's unbearably
slow if you have a lot of tags.
> > Looking at this, I'm pretty sure that using "--contains" with "--merged"
> > has similar problems, as they both use the UNINTERESTING bit. So even
> > without your patch, there is a lurking bug.
>
> I'm currently running this:
> https://gist.github.com/avar/45cf288ce7cdc43e7395c6cbf9a98d68
Cute. I'll be curious if it turns up anything.
-Peff
^ permalink raw reply
* Re: [PATCH 01/10] pack-objects: eat CR in addition to LF after fgets.
From: Jeff Hostetler @ 2017-03-09 15:46 UTC (permalink / raw)
To: Jeff King, Jeff Hostetler; +Cc: git, gitster, markbt, benpeart, jonathantanmy
In-Reply-To: <20170309070128.we6hbraea5am3ado@sigill.intra.peff.net>
On 3/9/2017 2:01 AM, Jeff King wrote:
> On Wed, Mar 08, 2017 at 05:37:56PM +0000, Jeff Hostetler wrote:
>
>> From: Jeff Hostetler <git@jeffhostetler.com>
>>
>> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
>> ---
>> builtin/pack-objects.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>> index f294dcf..7e052bb 100644
>> --- a/builtin/pack-objects.c
>> +++ b/builtin/pack-objects.c
>> @@ -2764,6 +2764,8 @@ static void get_object_list(int ac, const char **av)
>> int len = strlen(line);
>> if (len && line[len - 1] == '\n')
>> line[--len] = 0;
>> + if (len && line[len - 1] == '\r')
>> + line[--len] = 0;
>
> Rather than add features to this bespoke line-reader, can we switch this
> to use strbuf_getline()? That handles line endings, and avoids the
> awkward corner case where fgets "breaks" a long line across two calls.
>
> Something like the patch below. I suspect read_object_list_from_stdin()
> should get the same treatment.
Much nicer. Will do. Thanks!
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 76b1919ca..6b9fffe9c 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2765,7 +2765,7 @@ static void record_recent_commit(struct commit *commit, void *data)
> static void get_object_list(int ac, const char **av)
> {
> struct rev_info revs;
> - char line[1000];
> + struct strbuf buf = STRBUF_INIT;
> int flags = 0;
>
> init_revisions(&revs, NULL);
> @@ -2775,12 +2775,12 @@ static void get_object_list(int ac, const char **av)
> /* make sure shallows are read */
> is_repository_shallow();
>
> - while (fgets(line, sizeof(line), stdin) != NULL) {
> - int len = strlen(line);
> - if (len && line[len - 1] == '\n')
> - line[--len] = 0;
> - if (!len)
> + while (strbuf_getline(&buf, stdin) != EOF) {
> + const char *line = buf.buf;
> +
> + if (!buf.len)
> break;
> +
> if (*line == '-') {
> if (!strcmp(line, "--not")) {
> flags ^= UNINTERESTING;
> @@ -2800,6 +2800,7 @@ static void get_object_list(int ac, const char **av)
> if (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))
> die("bad revision '%s'", line);
> }
> + strbuf_release(&buf);
>
> if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
> return;
>
^ permalink raw reply
* Re: diff.ignoreSubmoudles config setting broken?
From: Sebastian Schuberth @ 2017-03-09 16:15 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, Jacob Keller, Jeff King, Git Mailing List,
Jens Lehmann
In-Reply-To: <xmqq8tof32x9.fsf@gitster.mtv.corp.google.com>
On Wed, Mar 8, 2017 at 9:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Between these two:
>
> git -c diff.ignoresubmodules=all diff
> git diff --ignore-submodules=all
>
> one difference is that the latter disables reading extra config from
> .gitmodules (!) while the former makes the command honor it.
>
> This comes from aee9c7d6 ("Submodules: Add the new "ignore" config
> option for diff and status", 2010-08-06), which is ancient and
> predates even v2.0, so if you see problems with v2.12 or v2.11 but
> not with older ones, that would rule out this theory.
Thanks for reminding me of possible settings .gitmodules. Indeed I have
[submodule "scanners/scancode-toolkit"]
path = scanners/scancode-toolkit
url = https://github.com/sschuberth/scancode-toolkit.git
ignore = untracked
in .gitmodules, which explains why globally setting
"diff.ignoreSubmodules" to "all" had no effect.
--
Sebastian Schuberth
^ permalink raw reply
* Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error
From: René Scharfe @ 2017-03-09 16:33 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano
Cc: Brandon Williams, git, Stefan Beller, Jeff King
In-Reply-To: <alpine.DEB.2.20.1703091221440.3767@virtualbox>
Am 09.03.2017 um 12:24 schrieb Johannes Schindelin:
> While I would have agreed earlier that René's patch looks less intrusive,
> I have to point out that there would not have been any possible regression
> if the original patch had introduced the die_on_error parameter. It would
> have made the contract *obvious*.
>
> The nicer API made the contract unobvious, and that was the reason that
> the bug could hide.
You mean nicer in the sense of more forgiving, right?
The meaning of calls of a function with a die_on_error parameter are
only obvious if at least the name of that parameter is known. A
declaration without it would not suffice:
char *real_pathdup(const char *, int);
There are two possibilities that can't be distinguished by looking at
callers alone:
char *real_pathdup(const char *path, int die_on_error);
char *real_pathdup(const char *path, int gentle);
An enum can help, so that calls look something like this:
copy_or_death = real_pathdup(path, DIE_ON_ERROR);
copy = real_pathdup(path, IGNORE_ERRORS); if (copy) ...
For binary flags we can easily encode that information into the function
names and thus simplify the API:
copy_or_death = real_pathdup_or_die(path);
copy = real_pathdup_gently(path); if (copy) ...
And we can drop the suffix of the variant used overwhelmingly often, but
as you pointed out that relies on readers knowing that convention.
I'm sure Brandon will balance these concerns nicely in follow-up patches. ;)
René
^ permalink raw reply
* Re: [PATCH GSoC] Allow "-" as a short-hand for "@{-1}" in branch deletions
From: Stefan Beller @ 2017-03-09 17:47 UTC (permalink / raw)
To: Shuyang Shi; +Cc: git@vger.kernel.org
In-Reply-To: <0102015ab11ee091-f9f11bb5-559a-4c92-b5f6-9f7755e8f4b9-000000@eu-west-1.amazonses.com>
Welcome to the Git community!
On Wed, Mar 8, 2017 at 7:31 PM, Shuyang Shi <shuyang790@gmail.com> wrote:
> The "-" shorthand that stands for "the branch we were previously on",
> like we did for "git merge -" sometime after we introduced "git checkout -".
> Now I am introducing this shorthand to branch delete, i.e.
> "git branch -d -".
>
> More reference:
> https://public-inbox.org/git/7vppuewl6h.fsf@alter.siamese.dyndns.org/
Following that link:
> But there is a very commonly accepted long tradition for "-" to mean
> "read from the standard input", so we cannot reuse it to mean "the
> branch I was previously on" for every command without first making
> sure the command will never want to use "-" for the other common
> purpose.
This contradicts the introduction of "git branch -d -" to mean to delete
the last branch, but rather could mean "read from stdin which branches
to delete"? It would be nice if you could clarify in your commit message
which of both this is and how this fits into the big picture of "design
cleanliness".
>
> And this has been tested:
>
> Ivan:git Ivan$ (cd t; prove --timer --jobs 1 ./t3200-branch.sh)
> [00:21:26] ./t3200-branch.sh .. ok 12293 ms ( 0.04 usr 0.01 sys +
> 5.97 cusr 2.52 csys = 8.54 CPU)
> [00:21:39]
> All tests successful.
> Files=1, Tests=113, 13 wallclock secs ( 0.07 usr 0.02 sys +
> 5.97 cusr 2.52 csys = 8.58 CPU)
> Result: PASS
Thanks for being cautious when developing on Git. However this part
of the email would end up as part of the commit message. And as we expect
all commits that land eventually to not break tests, this information is better
put at a more non-permanent place, such as below the '---' line (where there is
also the built stat. For example see [1] how to have different message parts
(one permanent section and some chatter that is relevant for the process
at the moment)
Also for testing, the tests only ensure that the old behavior does not break;
but we'd want to make sure the new functionality doesn't break in the
future either,
which can be done best by writing a test as well for this functionality.
[1] https://public-inbox.org/git/xmqqvarj1kix.fsf_-_@gitster.mtv.corp.google.com/
and as a commit:
https://github.com/gitster/git/commit/83218867fbf6d27c78efe3cfba01790b2f1d15d4
> https://github.com/git/git/pull/337
Oh I see, you're using submitgit to communicate the patch to the mailing list.
I am not sure if it supports splitting up the message as I eluded to above.
IIRC some people use submitgit for the patch and then use a webmailer
(e.g. gmail) to send followup messages such as successful tests or what changed
to prior versions.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH] repack: Add options to preserve and prune old pack files
From: jmelvin @ 2017-03-09 17:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, nasserg, mfick, peff, sbeller
In-Reply-To: <xmqq4lz4968p.fsf@gitster.mtv.corp.google.com>
On 2017-03-07 13:33, Junio C Hamano wrote:
> James Melvin <jmelvin@codeaurora.org> writes:
>
>> These options are designed to prevent stale file handle exceptions
>> during git operations which can happen on users of NFS repos when
>> repacking is done on them. The strategy is to preserve old pack files
>> around until the next repack with the hopes that they will become
>> unreferenced by then and not cause any exceptions to running processes
>> when they are finally deleted (pruned).
>
> I find it a very sensible strategy to work around NFS, but it does
> not explain why the directory the old ones are moved to need to be
> configurable. It feels to me that a boolean that causes the old
> ones renamed s/^pack-/^old-&/ in the same directory (instead of
> pruning them right away) would risk less chances of mistakes (e.g.
> making "preserved" subdirectory on a separate device mounted there
> in a hope to reduce disk usage of the primary repository, which
> may defeat the whole point of moving the still-active file around
> instead of removing them).
Moving the preserved pack files to a separate directory only helped make
the pack directory cleaner, but I agree that having the old* pack files
in the same directory is a better approach as it would ensure that it's
still on the same mounted device. I'll update the logic to reflect that.
As for the naming convention of the preserved pack files, there is
already some logic to remove "old-" files in repack. Currently this is
the naming convention I have for them:
pack-<sha1>.old-<ext>
pack-7412ee739b8a20941aa1c2fd03abcc7336b330ba.old-pack
One advantage of that is the extension is no longer an expected one,
differentiating it from current pack files.
That said, if that is not a concern, I could prefix them with
"preserved" instead of "old" to differentiate them from the other logic
that cleans up "old-*". What are your thoughts on that?
preserved-<sha1>.<ext>
preserved-7412ee739b8a20941aa1c2fd03abcc7336b330ba.pack
> And if we make "preserve-old" a boolean, perhaps the presence of
> "prune-preserved" would serve as a substitute for it, iow, perhaps
> we may only need --prune-preserved option (and repack.prunePreserved
> configuration variable)?
>
>> diff --git a/builtin/repack.c b/builtin/repack.c
>> index 677bc7c81..f1a0c97f3 100644
>> --- a/builtin/repack.c
>> +++ b/builtin/repack.c
>> @@ -10,8 +10,10 @@
>>
>> static int delta_base_offset = 1;
>> static int pack_kept_objects = -1;
>> +static int preserve_oldpacks = 0;
>> +static int prune_preserved = 0;
>
> We avoid initializing statics to 0 or NULL and instead let BSS take
> care of them...
Will do
>> static int write_bitmaps;
>> -static char *packdir, *packtmp;
>> +static char *packdir, *packtmp, *preservedir;
>
> ... just like what you did here.
>
>> @@ -108,6 +110,27 @@ static void get_non_kept_pack_filenames(struct s
>> ...
>> +static void preserve_pack(const char *file_path, const char
>> *file_name, const char *file_ext)
>> +{
>> + char *fname_old;
>> +
>> + if (mkdir(preservedir, 0700) && errno != EEXIST)
>> + error(_("failed to create preserve directory"));
>
> You do not want to do the rest of this function after issuing this
> error, no? Because ...
Agreed, I'll update.
>> +
>> + fname_old = mkpathdup("%s/%s.old-%s", preservedir, file_name,
>> ++file_ext);
>> + rename(file_path, fname_old);
>
> ... this rename(2) would fail, whose error return you would catch
> and act on.
>
>> + free(fname_old);
>> +}
>> +
>> +static void remove_preserved_dir(void) {
>> + struct strbuf buf = STRBUF_INIT;
>> +
>> + strbuf_addstr(&buf, preservedir);
>> + remove_dir_recursively(&buf, 0);
>
> This is a wrong helper function to use on files and directories
> inside .git/; the function is about removing paths in the working
> tree.
Will update.
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v2 10/11] submodule--helper init: set submodule.<name>.active
From: Brandon Williams @ 2017-03-09 17:56 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Junio C Hamano
In-Reply-To: <CAGZ79kYspNw4=dSnDtyWp7bEhggAM-gRo3g9GJ78jZ8HW7pUFQ@mail.gmail.com>
On 03/08, Stefan Beller wrote:
> On Wed, Mar 8, 2017 at 5:23 PM, Brandon Williams <bmwill@google.com> wrote:
> > When initializing a submodule set the submodule.<name>.active config to
> > true to indicate that the submodule is active.
>
> So by this patch an init of a submodule performs both
> a "regular init" (copy URL and the update setting)
> as well as setting the new submodule.<name>.active.
>
> And both happens in the same config file (i.e. no worktree
> support here)
Well there isn't any per-worktree config yet (unless that series managed
to land and I didn't notice that). So once their are per-worktree
configs we would need to push this to there.
>
> Later on we *could* remove the URL as the .active is the new
> flag of existence.
>
> But enough of my speculation, I am left wondering what this
> patch accomplishes, as this states no agenda as why it is useful
> on its own.
>
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> > builtin/submodule--helper.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index bceb62a20..44f2c02ba 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -329,6 +329,13 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
> > die(_("No url found for submodule path '%s' in .gitmodules"),
> > displaypath);
> >
> > + /* Set active flag for the submodule being initialized */
> > + if (!is_submodule_initialized(path)) {
> > + strbuf_reset(&sb);
> > + strbuf_addf(&sb, "submodule.%s.active", sub->name);
> > + git_config_set_gently(sb.buf, "true");
> > + }
> > +
> > /*
> > * Copy url setting when it is not set yet.
> > * To look up the url in .git/config, we must not fall back to
> > --
> > 2.12.0.246.ga2ecc84866-goog
> >
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v2 07/11] submodule update: add `--init-active` switch
From: Brandon Williams @ 2017-03-09 18:08 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Junio C Hamano
In-Reply-To: <CAGZ79kbyyJCZ7iP6c7RBgrv8NNSMjb9fov3eFazpOb2QL5aAcg@mail.gmail.com>
On 03/08, Stefan Beller wrote:
> On Wed, Mar 8, 2017 at 5:23 PM, Brandon Williams <bmwill@google.com> wrote:
> > The new switch `--init-active` initializes the submodules which are
> > configured in `submodule.active` instead of those given as
> > command line arguments before updating. In the first implementation this
> > is made incompatible with further command line arguments as it is
> > unclear what the user means by
> >
> > git submodule update --init --init-active <paths>
> >
> > This new switch allows users to record more complex patterns as it saves
> > retyping them whenever you invoke update.
> >
> > Based on a patch by Stefan Beller <sbeller@google.com>
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
>
>
>
> > @@ -568,7 +573,17 @@ cmd_update()
> >
> > if test -n "$init"
> > then
> > - cmd_init "--" "$@" || return
> > + if test "$init" = "by_config"
> > + then
> > + if test $# -gt 0
> > + then
> > + die "$(gettext "path arguments are incompatible with --init-active")"
> > + fi
> > + cmd_init "--" $(git config --get-all submodule.active) || return
>
> What happens with submodule.<name>.actives here?
Nothing? I am still not 100% certain that we really need the
submodule.<name>.active config but it seemed like what we decided upon
last we discussed this.
Maybe we don't even need this as an option? Maybe update should be able
to just 'update' all active submodules without having to make an
explicit call to cmd_init?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 03/10] pack-objects: test for --partial-by-size --partial-special
From: Johannes Sixt @ 2017-03-09 18:11 UTC (permalink / raw)
To: Jeff Hostetler
Cc: git, peff, gitster, markbt, benpeart, jonathantanmy,
Jeff Hostetler
In-Reply-To: <1488994685-37403-4-git-send-email-jeffhost@microsoft.com>
Am 08.03.2017 um 18:37 schrieb Jeff Hostetler:
> +test_expect_success 'setup' '
> + perl -e "print \"a\" x 11;" > a &&
> + perl -e "print \"a\" x 1100;" > b &&
> + perl -e "print \"a\" x 1100000;" > c &&
If the file contents do not matter, you can have the same without perl
like this:
printf "%011d" 0 >a &&
printf "%01100d" 0 >b &&
printf "%01100000d" 0 >c &&
-- Hannes
^ permalink raw reply
* git commit --interactive patch-mode no longer allows selecting files
From: Jörn Hees @ 2017-03-09 17:48 UTC (permalink / raw)
To: git
Hi,
i'm not entirely sure if this is a bug or intended (couldn't find it in the changelogs though)...
Before 2.12.0 a `git commit --int` / `git add --int` followed by [p] for patch-mode, would allow a numeric selection of the files to patch hunks for (this behavior is well documented all over the internet).
Since 2.12.0 the numeric selection is skipped and automatically drops me directly into hunk mode (Mac OS X homebrew).
I would see this as a feature if only one file is changed. As soon as multiple files are changed, i often have the use-case that i'm in some bigger change, but want to quickly fix something in other files. In those cases, the new behavior forces me to `d` out of potentially many files, just to quickly commit some small change :-/
I already tried in several repos and without ~/.gitconfig .
Are there any config options i'm missing?
Best,
Jörn
^ permalink raw reply
* Re: [PATCH v3 2/2] submodule--helper.c: remove duplicate code
From: Brandon Williams @ 2017-03-09 18:18 UTC (permalink / raw)
To: Valery Tolstov; +Cc: git, sbeller, gitster
In-Reply-To: <20170309012734.21541-3-me@vtolstov.org>
On 03/09, Valery Tolstov wrote:
> Remove code fragment from module_clone that duplicates functionality
> of connect_work_tree_and_git_dir in dir.c
>
> Signed-off-by: Valery Tolstov <me@vtolstov.org>
Looks good.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v2 0/2] Remove duplicate code from module_clone()
From: Brandon Williams @ 2017-03-09 18:19 UTC (permalink / raw)
To: Valery Tolstov; +Cc: git, sbeller, gitster
In-Reply-To: <20170309005636.20254-1-me@vtolstov.org>
On 03/09, Valery Tolstov wrote:
> > The usual protocol would be to rebase off of Stefan's series and build
> > on that (assuming you have a dependency against his series), indicating
> > that you are doing as such in your cover letter.
>
> So, should I send only my patch, or current format (patch and dependency)
> is acceptalbe?
What you did here should be fine. I was just pointing that out for future
series that you send out to the list.
--
Brandon Williams
^ permalink raw reply
* Re: git commit --interactive patch-mode no longer allows selecting files
From: Jeff King @ 2017-03-09 18:23 UTC (permalink / raw)
To: Jörn Hees; +Cc: git
In-Reply-To: <8C99B562-9194-4227-B40D-F64BBECEEE38@joernhees.de>
On Thu, Mar 09, 2017 at 06:48:58PM +0100, Jörn Hees wrote:
> i'm not entirely sure if this is a bug or intended (couldn't find it in the changelogs though)...
>
> Before 2.12.0 a `git commit --int` / `git add --int` followed by [p]
> for patch-mode, would allow a numeric selection of the files to patch
> hunks for (this behavior is well documented all over the internet).
>
> Since 2.12.0 the numeric selection is skipped and automatically drops
> me directly into hunk mode (Mac OS X homebrew).
It's a bug. The fix is in c852bd54bd87fdcdc825f5d45c26aa745be13ba6, but
has not yet been merged to any integration branches. I hope it will make
it into v2.12.1.
-Peff
^ permalink raw reply
* Re: [PATCH v2 03/11] submodule deinit: use most reliable url
From: Brandon Williams @ 2017-03-09 18:15 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Junio C Hamano
In-Reply-To: <CAGZ79kZG+Y+AtYYxeE5hmsNxfwmNp1h7aKAem=JD3yiKB6STPw@mail.gmail.com>
On 03/08, Stefan Beller wrote:
> On Wed, Mar 8, 2017 at 5:23 PM, Brandon Williams <bmwill@google.com> wrote:
> > The user could have configured the submodule to have a different URL
> > from the one in the superproject's config. To account for this read
> > what the submodule has configured for remote.origin.url and use that
> > instead.
>
> When reading this commit message, I first thought this is an unrelated
> bug fix. However it is not unrelated. In a later patch you introduce a mode
> where submodule.<name>.url may not exist in .git/config any more,
> (there may be a .existence flag instead?) such that url="", which is
> obviously a bad UI:
So the later patch doesn't actually omit submodule.<name>.url but we
could end up with this if you init and then deinit without actually
cloning the submodule. So maybe the best bet is to drop this patch from
the series and we can include one like it in a follow up which uses a
helper function that you suggest.
>
> Submodule '$name' (<empty string>) unregistered for path '$displaypath'"
>
> Like the first patch of this series, we could have a helper function
> "git submodule--helper url <name>" that figures out how to get the URL:
> 1) Look into that GIT_DIR
> 2) if non-existent check .git/config for the URL or
> 3) fall back to .gitmodules?
>
> Instead of having such a helper, we directly look into that first option
> hoping it exists, however it doesn't have to:
>
> git submodule init <ps>
> # no command in between
> git submodule deinit <ps>
> # submodule was not cloned yet, but we still test positive for
> > # Remove the .git/config entries (unless the user already did it)
> > if test -n "$(git config --get-regexp submodule."$name\.")"
>
> I am not sure if there is an easy way out here.
>
> Thinking about the name for such a url helper lookup, I wonder if
> we want to have
>
> git submodule--helper show-url <name>
>
> as potentially we end up having this issue for a lot
> of different things (e.g. submodule.<name>.shallow = (true,false),
> or in case the submodule is cloned you can give the actual depth
> as an integral number), so maybe we'd want to introduce one
> layer of indirection
>
> git submodule--helper ask-property \
> (is-active/URL/depth/size/..) <name>
>
> So with that said, I wonder if we also want to ease up:
>
> GIT_DIR="$(git rev-parse --git-path modules/$name
>
> to be
>
> GIT_DIR=$(git submodule--helper show-git-dir $name)
>
> > then
> > # Remove the whole section so we have a clean state when
> > # the user later decides to init this submodule again
> > - url=$(git config submodule."$name".url)
> > + url=$(GIT_DIR="$(git rev-parse --git-path modules/$name)" git config remote.origin.url)
>
> In the submodule helper we have get_default_remote(), which we do not
> have in shell
> (which we seemed to have in shell?), so maybe it's worth not using origin here,
> although I guess it is rare that the original remote is named other than origin.
>
> > git config --remove-section submodule."$name" 2>/dev/null &&
> > say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$displaypath'")"
> > fi
> > --
> > 2.12.0.246.ga2ecc84866-goog
> >
>
> Thanks,
> Stefan
--
Brandon Williams
^ 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