Re: [PATCH 1/3] t6501: use --quiet when testing gc stderr
"Derrick Stolee via GitGitGadget" writes: > From: Derrick Stolee > > The test script t6501-freshen-objects.sh has some tests that care > if 'git gc' has any output to stderr. This is intended to say that > no warnings occurred related to broken links. However, when we > have operations that output progress (like writing the commit-graph) > this causes the test to fail. I see that the descriptor #2 is redirected into a regular file. Why should we be writing the progress indicator in that case in the first place? Shoudln't we be doing the usual "are we showing this to an interactive terminal?" test?
Re: [PATCH] upload-pack: clear flags before each v2 request
Jonathan Tan writes: > Suppose a server has the following commit graph: > > A B > \ / >O > > We create a client by cloning A from the server with depth 1, and add > many commits to it (so that future fetches span multiple requests due to > lengthy negotiation). If it then fetches B using protocol v2, the fetch > spanning multiple requests, the resulting packfile does not contain O > even though the client did report that A is shallow. Hmph, what if commit O had a long history behind it? Should fetching of B result in fetching the whole history? Would we notice that now all of A's parents are available locally and declare that the repository is no longer shallow? I am trying to figure out if "does not contain O when we fetch B, even though we earlier fetched A shallowly, excluding its parents" is unconditionally a bad thing. The change to the code itself sort-of makes sense (I say sort-of because I didn't carefully look at the callers to see if they mind getting all these flags cleared, but the ones that are cleared are the ones that are involved mostly in the negotiation and shold be OK). > diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh > index 88a886975d..70b88385ba 100755 > --- a/t/t5702-protocol-v2.sh > +++ b/t/t5702-protocol-v2.sh > @@ -429,6 +429,31 @@ test_expect_success 'fetch supports include-tag and tag > following' ' > git -C client cat-file -e $(git -C client rev-parse annotated_tag) > ' > > +test_expect_success 'upload-pack respects client shallows' ' > + rm -rf server client trace && > + > + git init server && > + test_commit -C server base && > + test_commit -C server client_has && > + > + git clone --depth=1 "file://$(pwd)/server" client && > + > + # Add extra commits to the client so that the whole fetch takes more > + # than 1 request (due to negotiation) > + for i in $(seq 1 32) Use test_seq instead, or you'll get hit by test-lint? Applied on 'master' or 'maint', this new test does not pass even with s/seq/test_&/, so there may be something else wrong with it, though.
Re: [PATCH v2] list-objects: support for skipping tree traversal
Matthew DeVore writes: > The tree:0 filter does not need to traverse the trees that it has > filtered out, so optimize list-objects and list-objects-filter to skip > traversing the trees entirely. Before this patch, we iterated over all > children of the tree, and did nothing for all of them, which was > wasteful. > > Signed-off-by: Matthew DeVore > --- Thanks, will queue.
Re: [PATCH v2] headers: normalize the spelling of some header guards
Ramsay Jones writes: > Signed-off-by: Ramsay Jones > --- > > Hi Junio, > > Since I didn't get any adverse comments, this version has the RFC > label removed. Also, given that it seems the vcs-svn directory is > not going away soon, I have included those headers this time as well. > > [Note: my email client (thunderbird) was updated yesterday to 60.2.1. > As a result of recent reports, I sent this patch to myself and applied > it with 'git am' and ... the results seem to be fine! ;-) Famous last > words!] Thanks, will queue.
Re: [PATCH 0/3] Use commit-graph by default
"Derrick Stolee via GitGitGadget" writes: > The commit-graph feature is starting to stabilize. Based on what is in > master right now, we have: > > Git 2.18: > > * Ability to write commit-graph (requires user interaction). > > > * Commit parsing is faster when commit-graph exists. > > > * Must have core.commitGraph true to use. > > > > Git 2.19: > > * Ability to write commit-graph on GC with gc.writeCommitGraph. > > > * Generation numbers written in commit-graph > > > * A few reachability algorithms make use of generation numbers. > > > > (queued for) master: > > * The test suite passes with GIT_TEST_COMMIT_GRAPH=1 > > > * 'git commit-graph write' has progress indicators. > > > * The commit-graph is automatically disabled when grafts or replace-objects >exist. If I recall correctly, one more task that was discussed but hasn't been addressed well is how the generation and incremental update of it should integrate with the normal repository maintenance workflow (perhaps "gc --auto"). If we are going to turn it on by default, it would be good to see if we can avoid multiple independent walks done over the same history graph by repack, prune and now commit-graph, before such a change happens. Thanks.
Re: [PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet
Jeff King writes: > Whereas for the new config variable, you'd probably set it not because > you want it quiet all the time, but because you want to get some time > savings. So there it does make sense to me to explain. > > Other than that, this seems like an obvious and easy win. It does feel a > little hacky (you're really losing something in the output, and ideally > we'd just be able to give that answer quickly), but this may be OK as a > hack in the interim. After "git reset --quiet -- this/area/" with this change, any operation you'd do next that needs to learn if working tree files are different from what is recorded in the index outside that area will have to spend more cycles, because the refresh done by "reset" is now limited to the area. So if your final goal is "make 'reset' as fast as possible", this is an obvious and easy win. For other goals, i.e. "make the overall experience of using Git feel faster", it is not so obvious to me, though. If we somehow know that it is much less important in your setup that the cached stat bits in the index is kept up to date (e.g. perhaps you are more heavily relying on fsmonitor and are happy with it), then I suspect that we could even skip the refreshing altogether and gain more performance, without sacrificing the "overall experience of using Git" at all, which would be even better. > The sad thing is just that it's user-facing, so we have to respect it > forever. I almost wonder if there should be a global core.optimizeMessages > or something that tries to tradeoff less information for speed in all > commands, but makes no promises about which. Then a user with a big repo > who sets it once will get the benefit as more areas are identified (I > think "status" already has a similar case with ahead/behind)? And vice > versa, as some messages get faster to produce, they can be dropped from > that option. > > I dunno. Maybe that is a stupid idea, and people really do want to > control it on a per-message basis. > > -Peff
Re: [RFC] revision: Add --sticky-default option
Jeff King writes: > I'd probably call it something verbose and boring like > --use-default-with-uninteresting or --default-on-negative. > I dunno. These two names are improvement, but there needs a hint that the change we are interested in is to use default even when revs are given as long as ALL of them are negative ones. Which in turn means there is NO positive ones given. So perhaps "--use-default-without-any-positive". Having said that, I have to wonder how serious a breakage we are going to cause to established users and scripts if we made this change without any explicit option. After all, it would be rather obvious that people will get a history with some commits (or none at all) when they were expecting no output that the "default behaviour" has changed. I also wonder how would scripts take advantage of the current "defeat --default as soon as we see any rev, even a negative one"---in short, I am not sure if the theoretical regression this new "option" is trying to avoid is worth avoiding in the first place. Is there a way to say "usually this command has built-in --default=HEAD behaviour, but I am declining that" already, i.e. $ git log --no-default $REVS that will result in an empty set if we accept the change proposed here but make it unconditional? If so "This and future versions of Git will honor the --default even when there are other revisions given on the command line, as long as they are ALL negative ones. This is a backward incompatibile change, but you can update your scripts with '--no-default' if you do not like the new behaviour" in the release notes may be a viable alternative way forward. If there is no such way in the released versions of Git, then that would not work, and a strict opt-in like the approach taken by the proposed patch would become necessary.
Re: [PATCH 5/9] submodule.c: do not copy around submodule list
Jonathan Tan writes: >> By doing so we'll have access to the util pointer for longer that >> contains the commits that we need to fetch, which will be >> useful in a later patch. > > It seems that the main point of this patch is so that the OIDs be stored > in changed_submodule_names, because you need them in a later patch. If > so, I would have expected a commit title like "submodule: store OIDs in > changed_submodule_names". > > ... > This patch looks good, except that the commit title and message could > perhaps be clearer. Thanks for a thoughtful review; not just this step, but for your review comments on all the other steps in the series are very helpful.
Re: [PATCH 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip
Stefan Beller writes: > This is based on ao/submodule-wo-gitmodules-checked-out. Thanks. I had an impression that we were not entirely happy with the topic and are expecting a reroll, but let's hope that the part we expect to be updated won't have much overlaps. > A range-diff below shows how picking a different base changed the patches, > apart from that no further adjustments have been made. Thanks. Let's see how well this plays together with other topics in 'pu'.q
Re: [PATCH 1/1] subtree: make install targets depend on build targets
Jonathan Nieder writes: > The rule says > > install-html: html > $(INSTALL) -d -m 755 $(DESTDIR)$(htmldir) > $(INSTALL) -m 644 $^ $(DESTDIR)$(htmldir) > > and $^ substitutes to "html" after this change. Sorry about that. From: Junio C Hamano Date: Thu, 18 Oct 2018 11:07:17 +0900 Subject: [PATCH] Revert "subtree: make install targets depend on build targets" This reverts commit 744f7c4c314dc0e7816ac05520e8358c8318187a. These targets do depend on the fact that each prereq is explicitly listed via their use of $^, which I failed to notice, and broke the build. --- contrib/subtree/Makefile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contrib/subtree/Makefile b/contrib/subtree/Makefile index 4a10a020a0..6906aae441 100644 --- a/contrib/subtree/Makefile +++ b/contrib/subtree/Makefile @@ -69,11 +69,11 @@ install: $(GIT_SUBTREE) install-doc: install-man install-html -install-man: man +install-man: $(GIT_SUBTREE_DOC) $(INSTALL) -d -m 755 $(DESTDIR)$(man1dir) $(INSTALL) -m 644 $^ $(DESTDIR)$(man1dir) -install-html: html +install-html: $(GIT_SUBTREE_HTML) $(INSTALL) -d -m 755 $(DESTDIR)$(htmldir) $(INSTALL) -m 644 $^ $(DESTDIR)$(htmldir) @@ -98,4 +98,4 @@ clean: $(RM) $(GIT_SUBTREE) $(RM) *.xml *.html *.1 -.PHONY: FORCE man html install-man install-html +.PHONY: FORCE -- 2.19.1-450-ga4b8ab5363
Re: [PATCH 1/1] subtree: make install targets depend on build targets
Jonathan Nieder writes: >> -install-html: $(GIT_SUBTREE_HTML) >> +install-html: html > > This broke the build for me: > > make[2]: Entering directory '/build/git-2.19.1+next.20181016/contrib/subtree' > install -m 644 html > /build/git-2.19.1+next.20181016/debian/tmp/usr/share/doc/git/html > install: cannot stat 'html': No such file or directory > make[2]: *** [Makefile:78: install-html] Error 1 > > The rule says > > install-html: html > $(INSTALL) -d -m 755 $(DESTDIR)$(htmldir) > $(INSTALL) -m 644 $^ $(DESTDIR)$(htmldir) > > and $^ substitutes to "html" after this change. How was this patch > tested? Gah, that was silly of me.
Re: [PATCH 0/9] Offer to run CI/PR builds in Visual Studio Team Services
Ævar Arnfjörð Bjarmason writes: >> sites could do the same polling and mirroring. I am just too lazy >> to open a new account at yet another hosting site to add that for >> loop, but I may choose to when I am absolutely bored and nothing >> else to do ;-). > > Do you mind if I squat gitlab.com/git/git in the meantime (i.e. create > an org etc.) and have it mirror github.com/git/git?, I'll hand the Obviously somebody who is not even interested in obtaining an account would appreciate, not just "would not mind", if a trusted member in the community did that for the community ;-) Thanks.
Re: [PATCH 0/4] Bloom filter experiment
Ævar Arnfjörð Bjarmason writes: >> This is all to say: having a maximum size is good. 512 is big enough >> to cover _most_ commits, but not so big that we may store _really_ big >> filters. > > Makes sense. 512 is good enough to hardcode initially, but I couldn't > tell from briefly skimming the patches if it was possible to make this > size dynamic per-repo when the graph/filter is written. > > I.e. we might later add some discovery step where we look at N number of > commits at random, until we're satisfied that we've come up with some > average/median number of total (recursive) tree entries & how many tend > to be changed per-commit. > > I.e. I can imagine repositories (with some automated changes) where we > have 10k files and tend to change 1k per commit, or ones with 10k files > where we tend to change just 1-10 per commit, which would mean a > larger/smaller filter would be needed / would do. I was more interested to find out what the advice our docs should give to the end users to tune the value once such a knob is invented, and what you gave in the above paragraphs may lead us to a nice auto-tuning heuristics.
Re: Ignored files being silently overwritten when switching branches
Duy Nguyen writes: > Just fyi I also have some wip changes that add the forth "precious" > class in addition to tracked, untracked and ignored [1]. If someone > has time it could be another option to pick up. It is much more sensible than gaining the ability to express precious by trading away the ability to express expendable, I would think.
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
Johannes Schindelin writes: >> In any case, you can use "http..$variable" to say "I want the >> http.$variable to be in effect but only when I am talking to ". >> Does it make sense for this new variable, too? That is, does it >> benefit the users to be able to do something like this? >> >> [http] schannelCheckRevoke = no >> [http "https://microsoft.com/;] schannelCheckRevoke = yes >> >> I am guessing that the answer is yes. > > Frankly, I do not know. Does it hurt, though? I did not and I do not think it would. I was wondering if the ability to be able to specify these per destination is something very useful and deserves to be called out in the doc, together with ... >> I guess the same comment applies to the previous step, but I suspect >> that the code structure may not allow us to switch the SSL backend >> so late in the game (e.g. "when talking to microsoft, use schannel, >> but when talking to github, use openssl"). ... this bit. > Crucially, this fails. The short version is: this is good! Because it > means that Git used the OpenSSL backend, as clearly intended. > > > Why does it fail? > ... > So there may still be some polishing needed, but as long as people are not using the "per destination" thing, the code is already good? That is something we may want to document. Thanks.
Re: [PATCH 9/9] builtin/fetch: check for submodule updates for non branch fetches
> @@ -887,11 +887,14 @@ static int store_updated_refs(const char *raw_url,
> const char *remote_name,
> rc |= update_local_ref(ref, what, rm, ,
> summary_width);
> free(ref);
> - } else
> + } else {
> + check_for_new_submodule_commits(>old_oid);
Does this need to be guarded with a recurse_submodules check, just like
in update_local_ref()?
Also, this warrants a comment - this is here because there is some code
later that requires the new submodule commits to be registered, and the
other branch does not require it only because update_local_ref() calls
it.
> @@ -615,7 +615,7 @@ test_expect_success "fetch new commits on-demand when
> they are not reachable" '
> git update-ref refs/changes/2 $D &&
> (
> cd downstream &&
> - git fetch --recurse-submodules --recurse-submodules-default
> on-demand origin refs/changes/2:refs/heads/my_branch &&
> + git fetch --recurse-submodules origin refs/changes/2 &&
> git -C submodule cat-file -t $C &&
> git checkout --recurse-submodules FETCH_HEAD
> )
I think there should be a new test - we can tell from the code that just
because fetching to FETCH_HEAD works doesn't mean that fetching to a ref
works, and vice versa.
Also, can you make the test fetch 2 refs? So that we know that it works
with more than one.
Re: [PATCH 8/9] fetch: retry fetching submodules if needed objects were not fetched
> Currently when git-fetch is asked to recurse into submodules, it dispatches
> a plain "git-fetch -C " (with some submodule related options
> such as prefix and recusing strategy, but) without any information of the
> remote or the tip that should be fetched.
>
> This works surprisingly well in some workflows (such as using submodules
> as a third party library), while not so well in other scenarios, such
> as in a Gerrit topic-based workflow, that can tie together changes
> (potentially across repositories) on the server side. One of the parts
> of such a Gerrit workflow is to download a change when wanting to examine
> it, and you'd want to have its submodule changes that are in the same
> topic downloaded as well. However these submodule changes reside in their
> own repository in their own ref (refs/changes/).
It seems that the pertinent situation is when, in the superproject, you
fetch a commit (which may be the target of a ref, or an ancestor of the
target of a ref) that points to a submodule commit that was not fetched
by the default refspec-less fetch that you describe in the first
paragraph. I would just describe it as follows:
But this default fetch is not sufficient, as a newly fetched commit in
the superproject could point to a commit in the submodule that is not
in the default refspec. This is common in workflows like Gerrit's.
When fetching a Gerrit change under review (from refs/changes/??), the
commits in that change likely point to submodule commits that have not
been merged to a branch yet.
Another thing you need to clarify is what happens if the fetch-by-commit
fails. Right now, it seems that it will make the whole thing fail, which
might be a surprising change in behavior.
> Retry fetching a submodule by object name if the object id that the
> superproject points to, cannot be found.
I don't really think of this as a retry - the first time, you're
fetching the default refspec, and the second time, with a specific list
of object IDs. Also, be consistent in the usage of "object name" and
"object id" - as far as I know, they are the same thing.
> This retrying does not happen when the "git fetch" done at the
> superproject is not storing the fetched results in remote
> tracking branches (i.e. instead just recording them to
> FETCH_HEAD) in this step. A later patch will fix this.
The test stores the result in a normal branch, not a remote tracking
branch. Is storing in a normal branch required?
Also, do you know why this is required? A naive reading of the patch
leads me to believe that this should work even if merely fetching to
FETCH_HEAD.
> +struct get_next_submodule_task {
> + struct repository *repo;
> + const struct submodule *sub;
> + unsigned free_sub : 1; /* Do we need to free the submodule? */
> + struct oid_array *commits;
> +};
Firstly, I don't think "commits" needs to be a pointer.
Document at least "commits". As far as I can tell, if NULL (or empty if
you take my suggestion), this means that this task is the first pass for
this particular submodule and the fetch needs to be done using the
default refspec. Otherwise, this task is the second pass for this
particular submodule and the fetch needs to be done passing the
contained OIDs.
> +static const struct submodule *get_default_submodule(const char *path)
> +{
> + struct submodule *ret = NULL;
> + const char *name = default_name_or_path(path);
> +
> + if (!name)
> + return NULL;
> +
> + ret = xmalloc(sizeof(*ret));
> + memset(ret, 0, sizeof(*ret));
> + ret->path = name;
> + ret->name = name;
> +
> + return (const struct submodule *) ret;
> +}
What is a "default" submodule and why would you need one?
> + task = get_next_submodule_task_create(spf->r, ce->name);
> +
> + if (!task->sub) {
> + free(task);
> + continue;
> }
Will task->sub ever be NULL?
> + if (spf->retry_nr) {
> + struct get_next_submodule_task *task = spf->retry[spf->retry_nr
> - 1];
> + struct strbuf submodule_prefix = STRBUF_INIT;
> + spf->retry_nr--;
> +
> + strbuf_addf(_prefix, "%s%s/", spf->prefix,
> task->sub->path);
> +
> + child_process_init(cp);
> + prepare_submodule_repo_env_in_gitdir(>env_array);
> + cp->git_cmd = 1;
> + cp->dir = task->repo->gitdir;
> +
> + argv_array_init(>args);
> + argv_array_pushv(>args, spf->args.argv);
> + argv_array_push(>args, "on-demand");
This means that we need to trust that the last entry in spf->args can
take an "on-demand" parameter. Could we supply that argument here
explicitly instead?
> + argv_array_push(>args, "--submodule-prefix");
> + argv_array_push(>args, submodule_prefix.buf);
> +
> + /* NEEDSWORK: have get_default_remote from s--h */
What is s--h?
> +static int
[PATCH v2] list-objects: support for skipping tree traversal
The tree:0 filter does not need to traverse the trees that it has
filtered out, so optimize list-objects and list-objects-filter to skip
traversing the trees entirely. Before this patch, we iterated over all
children of the tree, and did nothing for all of them, which was
wasteful.
Signed-off-by: Matthew DeVore
---
list-objects-filter.c | 11 +--
list-objects-filter.h | 6 ++
list-objects.c | 5 -
t/t6112-rev-list-filters-objects.sh | 13 +
4 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/list-objects-filter.c b/list-objects-filter.c
index 09b2b05d54..765f3df3b0 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -102,9 +102,16 @@ static enum list_objects_filter_result filter_trees_none(
case LOFS_BEGIN_TREE:
case LOFS_BLOB:
- if (filter_data->omits)
+ if (filter_data->omits) {
oidset_insert(filter_data->omits, >oid);
- return LOFR_MARK_SEEN; /* but not LOFR_DO_SHOW (hard omit) */
+ /* _MARK_SEEN but not _DO_SHOW (hard omit) */
+ return LOFR_MARK_SEEN;
+ } else {
+ /*
+* Not collecting omits so no need to to traverse tree.
+*/
+ return LOFR_SKIP_TREE | LOFR_MARK_SEEN;
+ }
case LOFS_END_TREE:
assert(obj->type == OBJ_TREE);
diff --git a/list-objects-filter.h b/list-objects-filter.h
index a6f6b4990b..52b4a84da9 100644
--- a/list-objects-filter.h
+++ b/list-objects-filter.h
@@ -24,6 +24,11 @@ struct oidset;
* In general, objects should only be shown once, but
* this result DOES NOT imply that we mark it SEEN.
*
+ * _SKIP_TREE : Used in LOFS_BEGIN_TREE situation - indicates that
+ * the tree's children should not be iterated over. This
+ * is used as an optimization when all children will
+ * definitely be ignored.
+ *
* Most of the time, you want the combination (_MARK_SEEN | _DO_SHOW)
* but they can be used independently, such as when sparse-checkout
* pattern matching is being applied.
@@ -45,6 +50,7 @@ enum list_objects_filter_result {
LOFR_ZERO = 0,
LOFR_MARK_SEEN = 1<<0,
LOFR_DO_SHOW = 1<<1,
+ LOFR_SKIP_TREE = 1<<2,
};
enum list_objects_filter_situation {
diff --git a/list-objects.c b/list-objects.c
index c2c5dd98ce..c41cc80db5 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -11,6 +11,7 @@
#include "list-objects-filter-options.h"
#include "packfile.h"
#include "object-store.h"
+#include "trace.h"
struct traversal_context {
struct rev_info *revs;
@@ -184,7 +185,9 @@ static void process_tree(struct traversal_context *ctx,
if (base->len)
strbuf_addch(base, '/');
- if (!failed_parse)
+ if (r & LOFR_SKIP_TREE)
+ trace_printf("Skipping contents of tree %s...\n", base->buf);
+ else if (!failed_parse)
process_tree_contents(ctx, tree, base);
if ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn) {
diff --git a/t/t6112-rev-list-filters-objects.sh
b/t/t6112-rev-list-filters-objects.sh
index c2c2d5bea1..bb8ce66050 100755
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -281,6 +281,19 @@ test_expect_success 'verify tree:0 includes trees in
"filtered" output' '
test_cmp expected filtered_types
'
+# Make sure tree:0 does not iterate through any trees.
+
+test_expect_success 'filter a GIANT tree through tree:0' '
+ GIT_TRACE=1 git -C r3 rev-list \
+ --objects --filter=tree:0 HEAD 2>filter_trace &&
+ grep "Skipping contents of tree [.][.][.]" filter_trace >actual &&
+ # One line for each commit traversed.
+ test_line_count = 2 actual &&
+
+ # Make sure no other trees were considered besides the root.
+ ! grep "Skipping contents of tree [^.]" filter_trace
+'
+
# Delete some loose objects and use rev-list, but WITHOUT any filtering.
# This models previously omitted objects that we did not receive.
--
2.19.1.568.g152ad8e336-goog
Re: [RFC PATCH 1/3] list-objects: support for skipping tree traversal
On Sun, Oct 14, 2018 at 4:15 PM Junio C Hamano wrote:
>
> This step looks more like "ow, we could have done the tree:0 support
> that is in 'next' better" than a part of "here is a series to do
> tree:N for non zero value of N".
>
> If that is the case, I'd prefer to see this step polished enough
> before [2-3/3] of this RFC is worked on, so that we can merge the
> tree:0 (but not yet tree:N) support that is solid down to 'master'
> soonish.
That's fair. So I will prioritize this patch above the rest of the
patchset and send it separately in the future.
> OK, so "not collecting omits" is synonymous to "N==0, we aren't
> doing tree at any level", and at this point in the series before the
> support for N>0 is introduced, we'd always take this "else" clause
> because tree:0 is the only thing we support?
Actually "not collecting omits" and depth==0 are orthogonal concepts.
"Collect omits" is the logic needed to implement the
--filter-print-omitted flag on git rev-list. You can do this when
depth==0 (it will actually print all the trees and blobs recursively).
"Collect omits" is tested *with* tree:0 in test t6112 in the test
labeled 'verify tree:0 includes trees in "filtered" output'
IOW, both branches here are important even in this patch. If we are
collecting omits, then we can't skip the tree because omits collecting
is recursive. Otherwise, we *can* skip the tree.
But maybe printing omits should not be recursive? The decision was
never discussed. The code to not be recursive is simpler, because we
don't need this if/else. Recursiveness is counter-intuitive since we
would "skip" a tree and at the same time print its contents.
>
> Style: our modern style is to use {} around the body which is a
> single statement on the else clause when the body of the
> corresponding if clause needs {} around (and vice versa), so
>
Fixed, and I didn't realize I was supposed to be hugging "else" with
the curly braces. What you say is what CodingGuidelines says to do.
Thanks for pointing that out.
>
> Even when failed_parse==true, i.e. we found that the tree object's
> data cannot be understood, if we have skip-tree bit set, that means
> we do not care---we won't be descending into its children anyway.
>
Yes.
> > +# Make sure tree:0 does not iterate through any trees.
> > +
> > +test_expect_success 'filter a GIANT tree through tree:0' '
> > + GIT_TRACE=1 git -C r3 rev-list \
> > + --objects --filter=tree:0 HEAD 2>filter_trace &&
> > + grep "Skipping contents of tree [.][.][.]" filter_trace >actual &&
>
> Here you are not jus tmaking sure SKIP_TREE bit is set for some
> tree, but it is set when base->buf is an empty string (i.e. the top
> level tree)? Which makes sense, and the next text makes sure that
> between the two commits, the number of total "top level" trees is 2,
> but I wonder if it is more direct to also make sure that the code is
> not even seeing or skipping any tree inside these top level trees
> (i.e. the same message but for ""!=base->buf should never appear in
> the trace).
Makes sense. I added another check in this test for other "Skipping" messages.
Thank you for reviewing.
Re: [PATCH v2 13/13] commit-graph: specify OID version for SHA-256
On Wed, Oct 17, 2018 at 04:31:19PM +0200, Duy Nguyen wrote: > On Wed, Oct 17, 2018 at 12:44 AM brian m. carlson > wrote: > > Honestly, anything in the .git directory that is not the v3 pack indexes > > or the loose object file should be in exactly one hash algorithm. We > > could simply just leave this value at 1 all the time and ignore the > > field, since we already know what algorithm it will use. > > In this particular case, I agree, but not as a general principle. It's > nice to have independence for fsck-like tools. I don't know if we have > a tool that simply validates commit-graph file format (and not trying > to access any real object). But for such a tool, I guess we can just > pass the hash algorithm from command line. The user would have to > guess a bit. I'm going to drop this patch for now. I'll send a follow-up series later which bumps the format version for this and the multi-pack index and serializes them with the four-byte value. I probably should have caught this earlier, but unfortunately I don't always have the time to look at every series that hits the list. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH v2 03/13] hex: introduce functions to print arbitrary hashes
On Tue, Oct 16, 2018 at 10:54:23AM +0900, Junio C Hamano wrote: > Even though in hex.c I see mixture of *_algo and *_algop helper > functions, I see only "algo" variants above. Is it our official > stance to use primarily the integer index into the algo array when > specifying the hash, and when a caller into 'multi-hash' API happens > to have other things, it should use functions in 2/13 to convert it > to the canonical "int algo" beforehand? That was my intention, yes. > I am not saying it is bad or good to choose the index into the algo > array as the primary way to specify the algorithm. I think it is a > good idea to pick one and stick to it, and I wanted to make sure > that the choice we made here is clearly communicated to any future > developer who read this code. Yeah, that was my feeling as well. I wanted to pick something fixed and stick to it. > Having said the above, seeing the use of hash_algo_by_ptr() here > makes me suspect if it makes more sense to use the algop as the > primary way to specify which algorithm the caller wants to use. > IOW, making the set of helpers in 02/13 to allow quering by name, > format-id, or the integer index and have them all return a pointer > to "const struct git_hash_algo". Two immediate downsides I can see > is that it exposes the actual structure to the callers (but is it > really a problem? Outside callers learn hash sizes etc. by accessing > its fields anyway without treating the algo struct as opaque.), and > passing an 8-byte pointer may be more costly than passing a small > integer index that ranges between 0 and 1 at most (assuming that > we'd only use SHA-1 and "the current NewHash" in the code). I thought about this. The one downside to this is that we can't use those values anywhere we need an integer constant expression, like a switch. I suppose that just means we need hash_algo_by_ptr in those cases, and not everywhere else, which would make the code cleaner. Let me reroll with that change, and we'll see if we like it better. If we don't, I can pull the old version out of history. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH v2 09/13] git-p4: use `test_atexit` to kill the daemon
On Mon, Oct 15, 2018 at 03:12:11AM -0700, Johannes Schindelin via GitGitGadget
wrote:
> From: Johannes Schindelin
>
> This should be more reliable than the current method, and prepares the
> test suite for a consistent way to clean up before re-running the tests
> with different options.
This patch makes the test suite hang in all four Travis CI build jobs
with P4 installed without any of the P4 tests finishing.
Reverting this patch from the whole patch series makes it work again.
I've also tried to revert only this first hunk of the patch below,
because based on the comment I thought it's worth a try, but it didn't
really help. It did make a difference: the 300s watchdog timer
eventually kicked in, and then the test scripts could finish
successfully... but there are a lot of P4 test scripts, and with each
taking 300s the build job still timeouted.
All this may (or may not) be related to and be a different symptom of
the leftover p4d processes Luke mentioned. I couldn't reproduce any
of this on my machine so far.
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> index c27599474c..f4f5d7d296 100644
> --- a/t/lib-git-p4.sh
> +++ b/t/lib-git-p4.sh
> @@ -74,15 +74,6 @@ cli="$TRASH_DIRECTORY/cli"
> git="$TRASH_DIRECTORY/git"
> pidfile="$TRASH_DIRECTORY/p4d.pid"
>
> -# Sometimes "prove" seems to hang on exit because p4d is still running
> -cleanup () {
> - if test -f "$pidfile"
> - then
> - kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
> - fi
> -}
> -trap cleanup EXIT
> -
> # git p4 submit generates a temp file, which will
> # not get cleaned up if the submission fails. Don't
> # clutter up /tmp on the test machine.
Re: [PATCH v2 10/13] Add a base implementation of SHA-256 support
On Wed, Oct 17, 2018 at 06:12:41PM +0200, SZEDER Gábor wrote: > On macOS there is a MIN macro already defined in the system headers, > resulting in the following error: > > CC sha256/block/sha256.o > sha256/block/sha256.c:133:9: error: 'MIN' macro redefined > [-Werror,-Wmacro-redefined] > #define MIN(x, y) ((x) < (y) ? (x) : (y)) > ^ > /usr/include/sys/param.h:215:9: note: previous definition is here > #define MIN(a,b) (((a)<(b))?(a):(b)) > ^ > 1 error generated. > make: *** [sha256/block/sha256.o] Error 1 > > A simple "#undef MIN" solves this issue. However, I wonder whether we > should #undef the other #define directives as well, just to be sure > (and perhaps overly cautious). I'll undefine the macros at the top. For MIN, I'm going to go with the simple approach and pull pieces from the block-sha1 implementation, which doesn't require this code path... > Some GCC versions (e.g. gcc-4.8 with -O2 -Wall -Werror) complain about > the above line: > > CC sha256/block/sha256.o > sha256/block/sha256.c: In function ‘blk_SHA256_Final’: > sha256/block/sha256.c:174:2: error: dereferencing type-punned pointer will > break strict-aliasing rules [-Werror=strict-aliasing] > put_be64(ctx->buf + trip, ctx->length); > ^ > cc1: all warnings being treated as errors > make: *** [sha256/block/sha256.o] Error 1 > > Something like this makes it compile: > > void *ptr = ctx->buf + trip; > put_be64(ptr, ctx->length); > > However, it's not immediately obvious to me why the compiler > complains, or why that intermediate void* variable makes any > difference, but now it's not the time to put on my language lawyer > hat. > > Perhaps an old compiler bug? Clang in general, newer GCC versions, or > gcc-4.8 with -Wall -Werror but without -O2 don't seem to be affected. ...or this code path. Presumably GCC 4.8 and macOS are happy with the code that we already have in block-sha1. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH 7/9] submodule: fetch in submodules git directory instead of in worktree
> This patch started as a refactoring to make 'get_next_submodule' more
> readable, but upon doing so, I realized that "git fetch" of the submodule
> actually doesn't need to be run in the submodules worktree. So let's run
> it in its git dir instead.
The commit message needs to be updated, I think - this patch does
significantly more than fetching in the gitdir.
> This patch leaks the cp->dir in get_next_submodule, as any further
> callback in run_processes_parallel doesn't have access to the child
> process any more.
The cp->dir is already leaked - probably better to write "cp->dir in
get_next_submodule() is still leaked, but this will be fixed in a
subsequent patch".
> +static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)
> +{
> + prepare_submodule_repo_env_no_git_dir(out);
> + argv_array_pushf(out, "%s=.", GIT_DIR_ENVIRONMENT);
Why does GIT_DIR need to be set? Is it to avoid subcommands recursively
checking the parent directories in case the CWD is a malformed Git
repository? If yes, maybe it's worth adding a comment.
> +static struct repository *get_submodule_repo_for(struct repository *r,
> + const struct submodule *sub)
> +{
> + struct repository *ret = xmalloc(sizeof(*ret));
> +
> + if (repo_submodule_init(ret, r, sub)) {
> + /*
> + * No entry in .gitmodules? Technically not a submodule,
> + * but historically we supported repositories that happen to be
> + * in-place where a gitlink is. Keep supporting them.
> + */
> + struct strbuf gitdir = STRBUF_INIT;
> + strbuf_repo_worktree_path(, r, "%s/.git", sub->path);
> + if (repo_init(ret, gitdir.buf, NULL)) {
> + strbuf_release();
> + return NULL;
> + }
> + strbuf_release();
> + }
> +
> + return ret;
> +}
This is the significant thing that this patch does more - an unskipped
submodule is now something that either passes the checks in
repo_submodule_init() or the checks in repo_init(), which seems to be
stricter than the current check that ".git" points to a directory or is
one. This means that we skip certain broken repositories, and this
necessitates a change in the test.
I think we should be more particular about what we're allowed to skip -
in particular, maybe if we're planning to skip this submodule, its
corresponding directory in the worktree (if one exists) needs to be
empty.
> - cp->dir = strbuf_detach(_path, NULL);
> - prepare_submodule_repo_env(>env_array);
> + prepare_submodule_repo_env_in_gitdir(>env_array);
> + cp->dir = xstrdup(repo->gitdir);
Here is where the functionality change (fetch in ".git") described in
the commit message occurs.
Re: [PATCH v2 01/13] sha1-file: rename algorithm to "sha1"
On Tue, Oct 16, 2018 at 05:17:53PM +0200, Duy Nguyen wrote:
> On Mon, Oct 15, 2018 at 4:21 AM brian m. carlson
> wrote:
> >
> > The transition plan anticipates us using a syntax such as "^{sha1}" for
> > disambiguation. Since this is a syntax some people will be typing a
> > lot, it makes sense to provide a short, easy-to-type syntax. Omitting
> > the dash doesn't create any ambiguity, but it does make it shorter and
>
> "but" or "and"? I think both clauses are on the same side ... or did
> you mean omitting the dash does create ambiguity?
I think "but" is correct here. This is a standard "This doesn't make it
worse, but it does make it better" phrase. The "but" creates a contrast
between what it doesn't do and what it does.
I'm trying to come up with a different way to say this that may be
easier to understand, but I'm failing to do so in a natural-sounding
way. Does the following sound better?
Omitting the dash doesn't introduce any ambiguity; however, it does
make the text shorter and easier to type, especially for touch
typists.
> > easier to type, especially for touch typists. In addition, the
> > transition plan already uses "sha1" in this context.
> >
> > Rename the name of SHA-1 implementation to "sha1".
> >
> > Note that this change creates no backwards compatibility concerns, since
> > we haven't yet used this field in any serialized data formats.
>
> But we're not going to use this _string_ in any data format either
> because we'll stick to format_id field anyway, right?
We'll use it in extensions.objectFormat and other config files. But in
anything binary, we'll use format_id.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
Re: [PATCH v2 13/13] commit-graph: specify OID version for SHA-256
On Wed, Oct 17, 2018 at 08:21:42AM -0400, Derrick Stolee wrote:
> On 10/14/2018 10:19 PM, brian m. carlson wrote:
> > Since the commit-graph code wants to serialize the hash algorithm into
> > the data store, specify a version number for each supported algorithm.
> > Note that we don't use the values of the constants themselves, as they
> > are internal and could change in the future.
> >
> > Signed-off-by: brian m. carlson
> > ---
> > commit-graph.c | 9 -
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/commit-graph.c b/commit-graph.c
> > index 7a28fbb03f..e587c21bb6 100644
> > --- a/commit-graph.c
> > +++ b/commit-graph.c
> > @@ -45,7 +45,14 @@ char *get_commit_graph_filename(const char *obj_dir)
> > static uint8_t oid_version(void)
> > {
> > - return 1;
> > + switch (hash_algo_by_ptr(the_hash_algo)) {
> hash_algo_by_ptr is specified as an inline function in hash.h, so this leads
> to a linking error in jch and pu right now.
>
> I think the fix is simply to add '#include "hash.h"' to the list of
> includes.
hash.h is already included by cache.h, so it should be present. We
probably need to make it static. I'll make that change; can you see if
it fixes the problem for you?
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
[PATCH v2] headers: normalize the spelling of some header guards
Signed-off-by: Ramsay Jones --- Hi Junio, Since I didn't get any adverse comments, this version has the RFC label removed. Also, given that it seems the vcs-svn directory is not going away soon, I have included those headers this time as well. [Note: my email client (thunderbird) was updated yesterday to 60.2.1. As a result of recent reports, I sent this patch to myself and applied it with 'git am' and ... the results seem to be fine! ;-) Famous last words!] Thanks. ATB, Ramsay Jones alias.h | 4 ++-- commit-reach.h | 4 ++-- fetch-negotiator.h | 4 ++-- midx.h | 4 ++-- t/helper/test-tool.h | 4 ++-- vcs-svn/fast_export.h| 4 ++-- vcs-svn/line_buffer.h| 4 ++-- vcs-svn/sliding_window.h | 4 ++-- vcs-svn/svndiff.h| 4 ++-- vcs-svn/svndump.h| 4 ++-- 10 files changed, 20 insertions(+), 20 deletions(-) diff --git a/alias.h b/alias.h index 79933f2457..aef4843bb7 100644 --- a/alias.h +++ b/alias.h @@ -1,5 +1,5 @@ -#ifndef __ALIAS_H__ -#define __ALIAS_H__ +#ifndef ALIAS_H +#define ALIAS_H struct string_list; diff --git a/commit-reach.h b/commit-reach.h index 7d313e2975..122a23a24d 100644 --- a/commit-reach.h +++ b/commit-reach.h @@ -1,5 +1,5 @@ -#ifndef __COMMIT_REACH_H__ -#define __COMMIT_REACH_H__ +#ifndef COMMIT_REACH_H +#define COMMIT_REACH_H #include "commit-slab.h" diff --git a/fetch-negotiator.h b/fetch-negotiator.h index ddb44a22dc..9e3967ce66 100644 --- a/fetch-negotiator.h +++ b/fetch-negotiator.h @@ -1,5 +1,5 @@ -#ifndef FETCH_NEGOTIATOR -#define FETCH_NEGOTIATOR +#ifndef FETCH_NEGOTIATOR_H +#define FETCH_NEGOTIATOR_H struct commit; diff --git a/midx.h b/midx.h index ce80b91c68..ee83702309 100644 --- a/midx.h +++ b/midx.h @@ -1,5 +1,5 @@ -#ifndef __MIDX_H__ -#define __MIDX_H__ +#ifndef MIDX_H +#define MIDX_H #include "repository.h" diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index e4890566da..71f470b871 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -1,5 +1,5 @@ -#ifndef __TEST_TOOL_H__ -#define __TEST_TOOL_H__ +#ifndef TEST_TOOL_H +#define TEST_TOOL_H #include "git-compat-util.h" diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h index 60b79c35b9..9dcf9337c1 100644 --- a/vcs-svn/fast_export.h +++ b/vcs-svn/fast_export.h @@ -1,5 +1,5 @@ -#ifndef FAST_EXPORT_H_ -#define FAST_EXPORT_H_ +#ifndef FAST_EXPORT_H +#define FAST_EXPORT_H struct strbuf; struct line_buffer; diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h index ee23b4f490..e192aedea2 100644 --- a/vcs-svn/line_buffer.h +++ b/vcs-svn/line_buffer.h @@ -1,5 +1,5 @@ -#ifndef LINE_BUFFER_H_ -#define LINE_BUFFER_H_ +#ifndef LINE_BUFFER_H +#define LINE_BUFFER_H #include "strbuf.h" diff --git a/vcs-svn/sliding_window.h b/vcs-svn/sliding_window.h index b43a825cba..189c32d84c 100644 --- a/vcs-svn/sliding_window.h +++ b/vcs-svn/sliding_window.h @@ -1,5 +1,5 @@ -#ifndef SLIDING_WINDOW_H_ -#define SLIDING_WINDOW_H_ +#ifndef SLIDING_WINDOW_H +#define SLIDING_WINDOW_H #include "strbuf.h" diff --git a/vcs-svn/svndiff.h b/vcs-svn/svndiff.h index 74eb464bab..10a2cbc40e 100644 --- a/vcs-svn/svndiff.h +++ b/vcs-svn/svndiff.h @@ -1,5 +1,5 @@ -#ifndef SVNDIFF_H_ -#define SVNDIFF_H_ +#ifndef SVNDIFF_H +#define SVNDIFF_H struct line_buffer; struct sliding_view; diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h index b8eb12954e..26faed5968 100644 --- a/vcs-svn/svndump.h +++ b/vcs-svn/svndump.h @@ -1,5 +1,5 @@ -#ifndef SVNDUMP_H_ -#define SVNDUMP_H_ +#ifndef SVNDUMP_H +#define SVNDUMP_H int svndump_init(const char *filename); int svndump_init_fd(int in_fd, int back_fd); -- 2.19.0
Re: [PATCH 6/9] repository: repo_submodule_init to take a submodule struct
> When constructing a struct repository for a submodule for some revision > of the superproject where the submodule is not contained in the index, > it may not be present in the working tree currently either. In that > siutation giving a 'path' argument is not useful. Upgrade the > repo_submodule_init function to take a struct submodule instead. Are there ways for other code to create a struct submodule without using submodule_from_path()? If yes, maybe outline them here and say that this makes repo_submodule_init() more useful, since it can now be used with those methods. > While we are at it, overhaul the repo_submodule_init function by renaming > the submodule repository struct, which is to be initialized, to a name > that is not confused with the struct submodule as easily. "Overhaul" is probably not the right word for just a rename of a local variable. Also mention the other functions in which you did this rename (or just say "repo_submodule_init() and other functions"). > +/* > + * Initialize the repository 'subrepo' as the submodule given by the > + * struct submodule 'sub' in parent repository 'superproject'. > + * Return 0 upon success and a non-zero value upon failure. > + */ > +struct submodule; > +int repo_submodule_init(struct repository *subrepo, > struct repository *superproject, > - const char *path); > + const struct submodule *sub); >From this description, I would expect "sub" to not be allowed to be NULL, but from the code I don't think that's the case. Should we prohibit NULL (and add checks to all repo_submodule_init()'s callers) or document that a NULL sub is allowed (and what happens in that case)?
Re: [PATCH 5/9] submodule.c: do not copy around submodule list
> By doing so we'll have access to the util pointer for longer that
> contains the commits that we need to fetch, which will be
> useful in a later patch.
It seems that the main point of this patch is so that the OIDs be stored
in changed_submodule_names, because you need them in a later patch. If
so, I would have expected a commit title like "submodule: store OIDs in
changed_submodule_names".
> @@ -1142,8 +1142,7 @@ static void calculate_changed_submodule_paths(
> struct submodule_parallel_fetch *spf)
> {
> struct argv_array argv = ARGV_ARRAY_INIT;
> - struct string_list changed_submodules = STRING_LIST_INIT_DUP;
> - const struct string_list_item *name;
> + struct string_list_item *name;
Prior to this patch, changed_submodules is here just so that we know
what to add to changed_submodule_names (it will be cleared at the end of
the function). So removing it is fine.
> - collect_changed_submodules(_submodules, );
> + collect_changed_submodules(>changed_submodule_names, );
>
> - for_each_string_list_item(name, _submodules) {
> + for_each_string_list_item(name, >changed_submodule_names) {
We add all the changed submodules directly to changed_submodule_names,
and iterate over it. So we use changed_submodule_names as a
scratchpad...
> - if (!submodule_has_commits(path, commits))
> - string_list_append(>changed_submodule_names,
> -name->string);
> + if (submodule_has_commits(path, commits)) {
> + oid_array_clear(commits);
> + *name->string = '\0';
> + }
...but this is fine because previously, we appended the name->string to
changed_submodule_names (with no util) whereas now, we make the
name->string empty in the opposite condition.
Before this patch, at the end of the loop, we had all the wanted
submodule names with no util in changed_submodule_names. With this
patch, at the end of the loop, we have all the wanted submodule names
with util pointing to an OID array, and also some blanks with util
pointing to a cleared OID array.
> - free_submodules_oids(_submodules);
> + string_list_remove_empty_items(>changed_submodule_names, 1);
The local variable changed_submodules no longer exists, so removing that
line is fine.
And we remove all the blanks from changed_submodule_names.
> @@ -1377,7 +1378,7 @@ int fetch_populated_submodules(struct repository *r,
>
> argv_array_clear();
> out:
> - string_list_clear(_submodule_names, 1);
> + free_submodules_oids(_submodule_names);
And because changed_submodule_names now has a non-trivial util pointer,
we need to free it properly.
This patch looks good, except that the commit title and message could
perhaps be clearer.
Re: [PATCH 4/9] submodule.c: move global changed_submodule_names into fetch submodule struct
> The `changed_submodule_names` are only used for fetching, so let's make it > part of the struct that is passed around for fetching submodules. Keep the titles of commit messages to 50 characters or under. > +static void calculate_changed_submodule_paths( > + struct submodule_parallel_fetch *spf) Instead of taking the entire struct, could this just take the list of changed submodule names instead?
Re: [PATCH 3/9] submodule.c: sort changed_submodule_names before searching it
> We can string_list_insert() to maintain sorted-ness of the > list as we find new items, or we can string_list_append() to > build an unsorted list and sort it at the end just once. This confused me at first, because neither function is mentioned in the patch. > As we do not rely on the sortedness while building the > list, we pick the "append and sort at the end" as it > has better worst case execution times. It took me some time to see that you were rejecting the two solutions you listed in the first paragraph, and are instead using a third (that you describe in this paragraph). The code itself looks fine. In the future, I think that it's better if this type of patch went into its own patch set - this seems independent of the concerns of this patch set, so splitting up keeps patch sets small.
Re: Git wire protocol v2 fails fetching shallow changes with `pack has XXX unresolved deltas` on large repos with lots of tags Inbox x
> No changes are needed in mirrored repository. Crash happens both with > 2.18.0 and 2.19.1 git versions. Having repository locally is not > required but reduces test runtime, you can quite reliably reproduce > issue when fetching over net directly from chromium.orgbypassing > mirroring step. Do you have the reproduction steps for this? If I run git -c protocol.version=2 fetch --tags \ https://chromium.googlesource.com/chromium/src \ +refs/heads/*:refs/remotes/origin/* --depth=1 repeatedly in the same repository, it succeeds. (And I've checked with GIT_TRACE_PACKET that it uses protocol v2.)
[PATCH 3/3] commit-graph: Use commit-graph by default
From: Derrick Stolee
The config setting "core.commitGraph" enables using the commit-graph
file to accelerate commit walks through parsing speed and generation
numbers. The setting "gc.writeCommitGraph" enables writing the
commit-graph file on every non-trivial 'git gc' operation. Together,
they help users automatically improve their performance.
By setting these config variables to true by default, we make the
commit-graph feature an "opt-out" feature instead of "opt-in".
Signed-off-by: Derrick Stolee
---
Documentation/config.txt | 4 ++--
builtin/gc.c | 2 +-
commit-graph.c | 6 +++---
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index f6f4c21a54..dc5ee7c145 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -923,7 +923,7 @@ the `GIT_NOTES_REF` environment variable. See
linkgit:git-notes[1].
core.commitGraph::
If true, then git will read the commit-graph file (if it exists)
- to parse the graph structure of commits. Defaults to false. See
+ to parse the graph structure of commits. Defaults to true. See
linkgit:git-commit-graph[1] for more information.
core.useReplaceRefs::
@@ -1639,7 +1639,7 @@ gc.writeCommitGraph::
If true, then gc will rewrite the commit-graph file when
linkgit:git-gc[1] is run. When using linkgit:git-gc[1]
'--auto' the commit-graph will be updated if housekeeping is
- required. Default is false. See linkgit:git-commit-graph[1]
+ required. Default is true. See linkgit:git-commit-graph[1]
for details.
gc.logExpiry::
diff --git a/builtin/gc.c b/builtin/gc.c
index 871a56f1c5..77e7413f94 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -41,7 +41,7 @@ static int aggressive_depth = 50;
static int aggressive_window = 250;
static int gc_auto_threshold = 6700;
static int gc_auto_pack_limit = 50;
-static int gc_write_commit_graph;
+static int gc_write_commit_graph = 1;
static int detach_auto = 1;
static timestamp_t gc_log_expire_time;
static const char *gc_log_expire = "1.day.ago";
diff --git a/commit-graph.c b/commit-graph.c
index a686758603..a459272466 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -232,15 +232,15 @@ static int prepare_commit_graph(struct repository *r)
{
struct alternate_object_database *alt;
char *obj_dir;
- int config_value;
+ int config_value = 1;
if (r->objects->commit_graph_attempted)
return !!r->objects->commit_graph;
r->objects->commit_graph_attempted = 1;
+ repo_config_get_bool(r, "core.commitgraph", _value);
if (!git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) &&
- (repo_config_get_bool(r, "core.commitgraph", _value) ||
- !config_value))
+ !config_value)
/*
* This repository is not configured to use commit graphs, so
* do not load one. (But report commit_graph_attempted anyway
--
gitgitgadget
[PATCH 1/3] t6501: use --quiet when testing gc stderr
From: Derrick Stolee The test script t6501-freshen-objects.sh has some tests that care if 'git gc' has any output to stderr. This is intended to say that no warnings occurred related to broken links. However, when we have operations that output progress (like writing the commit-graph) this causes the test to fail. Use 'git gc --quiet' to avoid these progress indicators from causing a test failure. Signed-off-by: Derrick Stolee --- t/t6501-freshen-objects.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t6501-freshen-objects.sh b/t/t6501-freshen-objects.sh index 033871ee5f..0973130f06 100755 --- a/t/t6501-freshen-objects.sh +++ b/t/t6501-freshen-objects.sh @@ -137,7 +137,7 @@ test_expect_success 'do not complain about existing broken links (commit)' ' some message EOF commit=$(git hash-object -t commit -w broken-commit) && - git gc 2>stderr && + git gc --quiet 2>stderr && verbose git cat-file -e $commit && test_must_be_empty stderr ' @@ -147,7 +147,7 @@ test_expect_success 'do not complain about existing broken links (tree)' ' 100644 blob 0003foo EOF tree=$(git mktree --missing stderr && + git gc --quiet 2>stderr && git cat-file -e $tree && test_must_be_empty stderr ' @@ -162,7 +162,7 @@ test_expect_success 'do not complain about existing broken links (tag)' ' this is a broken tag EOF tag=$(git hash-object -t tag -w broken-tag) && - git gc 2>stderr && + git gc --quiet 2>stderr && git cat-file -e $tag && test_must_be_empty stderr ' -- gitgitgadget
[PATCH 0/3] Use commit-graph by default
The commit-graph feature is starting to stabilize. Based on what is in master right now, we have: Git 2.18: * Ability to write commit-graph (requires user interaction). * Commit parsing is faster when commit-graph exists. * Must have core.commitGraph true to use. Git 2.19: * Ability to write commit-graph on GC with gc.writeCommitGraph. * Generation numbers written in commit-graph * A few reachability algorithms make use of generation numbers. (queued for) master: * The test suite passes with GIT_TEST_COMMIT_GRAPH=1 * 'git commit-graph write' has progress indicators. * The commit-graph is automatically disabled when grafts or replace-objects exist. There are some other things coming that are in review (like 'git log --graph' speedups), but it is probably time to consider enabling the commit-graph by default. This series does that. For timing, I'm happy to leave this queued for a merge after the Git 2.20 release. There are enough things in master to justify not enabling this by default until that goes out and more people use it. PATCH 3/3 is rather simple, and is the obvious thing to do to achieve enabling these config values by default. PATCH 1/3 is a required change to make the test suite work with this change. This change isn't needed with GIT_TEST_COMMIT_GRAPH=1 because the commit-graph is up-to-date for these 'git gc' calls, so no progress is output. PATCH 2/3 is also a necessary evil, since we already had to disable GIT_TEST_COMMIT_GRAPH for some tests, we now also need to turn off core.commitGraph. Thanks, -Stolee Derrick Stolee (3): t6501: use --quiet when testing gc stderr t: explicitly turn off core.commitGraph as needed commit-graph: Use commit-graph by default Documentation/config.txt| 4 ++-- builtin/gc.c| 2 +- commit-graph.c | 6 +++--- t/t0410-partial-clone.sh| 3 ++- t/t5307-pack-missing-commit.sh | 3 ++- t/t6011-rev-list-with-bad-commit.sh | 3 ++- t/t6024-recursive-merge.sh | 3 ++- t/t6501-freshen-objects.sh | 6 +++--- 8 files changed, 17 insertions(+), 13 deletions(-) base-commit: a4b8ab5363a32f283a61ef3a962853556d136c0e Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-50%2Fderrickstolee%2Fcommit-graph-default-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-50/derrickstolee/commit-graph-default-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/50 -- gitgitgadget
[PATCH 2/3] t: explicitly turn off core.commitGraph as needed
From: Derrick Stolee There are a few tests that already require GIT_TEST_COMMIT_GRAPH=0 as they rely on an interaction with the commits in the object store that is circumvented by parsing commit information from the commit-graph instead. Before enabling core.commitGraph as true by default, explicitly turn the setting off for these tests. Signed-off-by: Derrick Stolee --- t/t0410-partial-clone.sh| 3 ++- t/t5307-pack-missing-commit.sh | 3 ++- t/t6011-rev-list-with-bad-commit.sh | 3 ++- t/t6024-recursive-merge.sh | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index cfd0655ea1..f5277fafb1 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -193,7 +193,8 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' ' git -C repo config core.repositoryformatversion 1 && git -C repo config extensions.partialclone "arbitrary string" && - GIT_TEST_COMMIT_GRAPH=0 git -C repo rev-list --exclude-promisor-objects --objects bar >out && + GIT_TEST_COMMIT_GRAPH=0 git -c core.commitGraph=false \ + -C repo rev-list --exclude-promisor-objects --objects bar >out && grep $(git -C repo rev-parse bar) out && ! grep $FOO out ' diff --git a/t/t5307-pack-missing-commit.sh b/t/t5307-pack-missing-commit.sh index dacb440b27..dc4c19d0aa 100755 --- a/t/t5307-pack-missing-commit.sh +++ b/t/t5307-pack-missing-commit.sh @@ -16,7 +16,8 @@ test_expect_success setup ' obj=$(git rev-parse --verify tag3) && fanout=$(expr "$obj" : "\(..\)") && remainder=$(expr "$obj" : "..\(.*\)") && - rm -f ".git/objects/$fanout/$remainder" + rm -f ".git/objects/$fanout/$remainder" && + git config core.commitGraph false ' test_expect_success 'check corruption' ' diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh index 545b461e51..da6949743d 100755 --- a/t/t6011-rev-list-with-bad-commit.sh +++ b/t/t6011-rev-list-with-bad-commit.sh @@ -42,7 +42,8 @@ test_expect_success 'corrupt second commit object' \ ' test_expect_success 'rev-list should fail' ' - test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list --all > /dev/null + test_must_fail env GIT_TEST_COMMIT_GRAPH=0 \ + git -c core.commitGraph=false rev-list --all > /dev/null ' test_expect_success 'git repack _MUST_ fail' \ diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh index 27c7de90ce..fccdf96f13 100755 --- a/t/t6024-recursive-merge.sh +++ b/t/t6024-recursive-merge.sh @@ -61,7 +61,8 @@ GIT_AUTHOR_DATE="2006-12-12 23:00:08" git commit -m F ' test_expect_success 'combined merge conflicts' ' - test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git merge -m final G + test_must_fail env GIT_TEST_COMMIT_GRAPH=0 \ + git -c core.commitGraph=false merge -m final G ' cat > expect << EOF -- gitgitgadget
Re: Sort output of diff --stat?
On Wed, Oct 17, 2018 at 02:21:57PM -0500, Robert Dailey wrote: > On Wed, Oct 17, 2018 at 1:47 PM Jeff King wrote: > > Hmm, I feel like another person asked for this recently, but I can't > > seem to find the thread. > > Is it this one? > https://www.mail-archive.com/git@vger.kernel.org/msg159212.html > > That's the only one I was able to find, but no one replied. Thanks for > your insight. I didn't get my hopes up, but maybe someday it'll be > added. Ah, yeah, that's the one. For some reason I thought I had replied, but obviously I did not. ;) -Peff
Re: Sort output of diff --stat?
On Wed, Oct 17, 2018 at 1:47 PM Jeff King wrote: > Hmm, I feel like another person asked for this recently, but I can't > seem to find the thread. Is it this one? https://www.mail-archive.com/git@vger.kernel.org/msg159212.html That's the only one I was able to find, but no one replied. Thanks for your insight. I didn't get my hopes up, but maybe someday it'll be added.
Re: Sort output of diff --stat?
On Wed, Oct 17, 2018 at 01:15:18PM -0500, Robert Dailey wrote: > I'd like to sort the output of `git diff --stat` such that files are > listed in descending order based on number of lines changed. The > closest solution I've found online[1] has several readability issues. > I'd rather see a built-in solution in git, if one exists. Can anyone > recommend a solution? Hmm, I feel like another person asked for this recently, but I can't seem to find the thread. Anyway, no, I don't think there's a solution short of parsing the output (you could parse --numstat, which is at least sane, but then you'd have to generate your own visual diffstat, which has a lot of corner cases). We have --orderfile, but that's about a static order based on filename. It seems reasonable to me for "--stat" to learn an option to sort (that would not affect, say, the total diff order, since sorting by line count makes very little sense for most other formats). -Peff
Re: commit-graph is cool (overcoming add_missing_tags() perf issues)
On Wed, Oct 17, 2018 at 11:00:03AM -0700, Elijah Newren wrote: > Digging in, almost all the time was CPU-bound and spent in > add_missing_tags()[2]. If I'm reading the code correctly, it appears > that function loops over each tag, calling in_merge_bases_many() once > per tag. Thus, for his case, we were potentially walking all of > history of the main branch 2.5k times. That seemed rather suboptimal. > > Before attempting to optimize, I decided to try out the commit-graph > with a version of git from pu. While I expected a speed-up, I was a > bit suprised that it was a factor of over 100; dropping the time for > local dry-run push[2] to sub-second. A quick look suggests that > commit-graph doesn't fix the fact that we call in_merge_bases_many() N > times from add_missing_tags() and thus likely need to do N merge base > computations, it just makes each of the N much faster. So, perhaps > there's still another scaling issue we'll eventually need to address, > but for now, I'm pretty excited about commit-graph. Yeah, I think this case would probably still benefit from an all-points traversal. This want to be basically the same as what "git tag --merged" is doing, I would think (see ref-filter.c:do_merge_filter). As an aside, it looks like do_merge_filter uses prepare_revision_walk(), which IIRC means that it is susceptible to wrong answers due to clock skew (basically because of the use of commit timestamps for traversal order). This seems like another place where generation numbers could make a quiet improvement. -Peff
Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting
On Wed, Oct 17, 2018 at 02:19:59PM -0400, Eric Sunshine wrote: > On Wed, Oct 17, 2018 at 12:40 PM Ben Peart wrote: > > Add a reset.quietDefault config setting that sets the default value of the > > --quiet flag when running the reset command. This enables users to change > > the default behavior to take advantage of the performance advantages of > > avoiding the scan for unstaged changes after reset. Defaults to false. > > As with the previous patch, my knee-jerk reaction is that this really > feels wrong being tied to --quiet. It's particularly unintuitive. > > What I _could_ see, and what would feel more natural is if you add a > new option (say, --optimize) which is more general, incorporating > whatever optimizations become available in the future, not just this > one special-case. A side-effect of --optimize is that it implies > --quiet, and that is something which can and should be documented. Heh, I just wrote something very similar elsewhere in the thread. I'm still not sure if it's a dumb idea, but at least we can be dumb together. -Peff
Re: [PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet
On Wed, Oct 17, 2018 at 02:14:32PM -0400, Eric Sunshine wrote: > > diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt > > @@ -95,7 +95,9 @@ OPTIONS > > --quiet:: > > - Be quiet, only report errors. > > + Be quiet, only report errors. Can optimize the performance of reset > > + by avoiding scaning all files in the repo looking for additional > > + unstaged changes. > > s/scaning/scanning/ > > However, I'm not convinced that this should be documented here or at > least in this fashion. When I read this new documentation before > reading the commit message, I was baffled by what it was trying to say > since --quiet'ness is a superficial quality, not an optimizer. My > knee-jerk reaction is that it doesn't belong in end-user documentation > at all since it's an implementation detail, however, I can see that > such knowledge could be handy for people in situations which would be > helped by this. That said, if you do document it, this doesn't feel > like the correct place to do so; it should be in a "Discussion" > section or something. (Who would expect to find --quiet documentation > talking about optimizations? Likely, nobody.) Yeah, I had the same thought. You'd probably choose --quiet because you want it, you know, quiet. Whereas for the new config variable, you'd probably set it not because you want it quiet all the time, but because you want to get some time savings. So there it does make sense to me to explain. Other than that, this seems like an obvious and easy win. It does feel a little hacky (you're really losing something in the output, and ideally we'd just be able to give that answer quickly), but this may be OK as a hack in the interim. The sad thing is just that it's user-facing, so we have to respect it forever. I almost wonder if there should be a global core.optimizeMessages or something that tries to tradeoff less information for speed in all commands, but makes no promises about which. Then a user with a big repo who sets it once will get the benefit as more areas are identified (I think "status" already has a similar case with ahead/behind)? And vice versa, as some messages get faster to produce, they can be dropped from that option. I dunno. Maybe that is a stupid idea, and people really do want to control it on a per-message basis. -Peff
Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting
On Wed, Oct 17, 2018 at 12:40 PM Ben Peart wrote: > Add a reset.quietDefault config setting that sets the default value of the > --quiet flag when running the reset command. This enables users to change > the default behavior to take advantage of the performance advantages of > avoiding the scan for unstaged changes after reset. Defaults to false. As with the previous patch, my knee-jerk reaction is that this really feels wrong being tied to --quiet. It's particularly unintuitive. What I _could_ see, and what would feel more natural is if you add a new option (say, --optimize) which is more general, incorporating whatever optimizations become available in the future, not just this one special-case. A side-effect of --optimize is that it implies --quiet, and that is something which can and should be documented. > Signed-off-by: Ben Peart
Re: commit-graph is cool (overcoming add_missing_tags() perf issues)
On 10/17/2018 2:00 PM, Elijah Newren wrote: Hi, Just wanted to give a shout-out for the commit-graph work and how impressive it is. I had an internal report from a user that git pushes containing only one new tiny commit were taking over a minute (in a moderate size repo with good network connectivity). After digging for a while, I noticed three unusual things about the repo[1]: * he had push.followTags set to true * upstream repo had about 20k tags (despite only 55k commits) * his repo had an additional 2.5k tags, but none of these were in the history of the branches he was pushing and thus would not be included in any pushes. Digging in, almost all the time was CPU-bound and spent in add_missing_tags()[2]. If I'm reading the code correctly, it appears that function loops over each tag, calling in_merge_bases_many() once per tag. Thus, for his case, we were potentially walking all of history of the main branch 2.5k times. That seemed rather suboptimal. Thanks for the report. I made a note to inspect add_missing_tags() for more improvement in the future. Before attempting to optimize, I decided to try out the commit-graph with a version of git from pu. While I expected a speed-up, I was a bit suprised that it was a factor of over 100; dropping the time for local dry-run push[2] to sub-second. A quick look suggests that commit-graph doesn't fix the fact that we call in_merge_bases_many() N times from add_missing_tags() and thus likely need to do N merge base computations, it just makes each of the N much faster. So, perhaps there's still another scaling issue we'll eventually need to address, but for now, I'm pretty excited about commit-graph. Without the commit-graph, you are getting a quadratic problem (N commits * T tags), but with the commit-graph you are also getting the benefit of generation numbers, so the "N commits" is actually likely _zero_ for most tags, because the tags have strictly lower generation number. In those cases, we can terminate without any walk at all. Thanks! -Stolee
Sort output of diff --stat?
I'd like to sort the output of `git diff --stat` such that files are listed in descending order based on number of lines changed. The closest solution I've found online[1] has several readability issues. I'd rather see a built-in solution in git, if one exists. Can anyone recommend a solution? [1]: https://gist.github.com/jakub-g/7599177
Re: [PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet
On Wed, Oct 17, 2018 at 12:40 PM Ben Peart wrote: > When git reset is run with the --quiet flag, don't bother finding any > additional unstaged changes as they won't be output anyway. This speeds up > the git reset command by avoiding having to lstat() every file looking for > changes that aren't going to be reported anyway. > > Signed-off-by: Ben Peart > --- > diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt > @@ -95,7 +95,9 @@ OPTIONS > --quiet:: > - Be quiet, only report errors. > + Be quiet, only report errors. Can optimize the performance of reset > + by avoiding scaning all files in the repo looking for additional > + unstaged changes. s/scaning/scanning/ However, I'm not convinced that this should be documented here or at least in this fashion. When I read this new documentation before reading the commit message, I was baffled by what it was trying to say since --quiet'ness is a superficial quality, not an optimizer. My knee-jerk reaction is that it doesn't belong in end-user documentation at all since it's an implementation detail, however, I can see that such knowledge could be handy for people in situations which would be helped by this. That said, if you do document it, this doesn't feel like the correct place to do so; it should be in a "Discussion" section or something. (Who would expect to find --quiet documentation talking about optimizations? Likely, nobody.)
Re: [RFC] revision: Add --sticky-default option
On Wed, Oct 17, 2018 at 03:53:41PM +0200, Andreas Gruenbacher wrote: > On Wed, 17 Oct 2018 at 11:12, Jeff King wrote: > > On Tue, Oct 16, 2018 at 11:24:38PM +0200, Andreas Gruenbacher wrote: > > > here's a long-overdue update of my proposal from August 29: > > > > > > [RFC] revision: Don't let ^ cancel out the default > > > > > > Does this look more acceptable that my first shot? > > > > I think it's going in the right direction. > > > > The name "--sticky-default" did not immediately make clear to me what it > > does. Is there some name that would be more obvious? > > It's the best I could think of. Better ideas, anyone? I'd probably call it something verbose and boring like --use-default-with-uninteresting or --default-on-negative. I dunno. -Peff
Re: [RFC] revision: Add --sticky-default option
On Wed, Oct 17, 2018 at 06:24:05AM -0700, Matthew DeVore wrote:
> > Yuck, t4202 is a mix of older and newer styles. I'm OK with this as-is
> > because you've matched the surrounding code, but these days I'd probably
> > write:
> >
> > test_expect_success '--sticky-default ^' '
> > {
> > echo sixth
> > echo fifth
> > } >expect &&
> > git log --format=%s --sticky-default ^HEAD~2 >actual &&
> > test_cmp expect actual
> > '
> >
>
> How about test_write_lines? That is a little more readable to me than
> the echos in a subshell. A patch was recently queued with a usage of
> that function:
Ah, yeah, that would be fine. I was trying to avoid a cat/here-doc combo
since it incurs a process, but test_write_lines is readable and fast.
The main style things I wanted to show are:
- setting up the expect file should go in the test_expect block
- no space between ">" and the filename
Those are both present in the surrounding code, but we're slowly
cleaning them up.
-Peff
commit-graph is cool (overcoming add_missing_tags() perf issues)
Hi, Just wanted to give a shout-out for the commit-graph work and how impressive it is. I had an internal report from a user that git pushes containing only one new tiny commit were taking over a minute (in a moderate size repo with good network connectivity). After digging for a while, I noticed three unusual things about the repo[1]: * he had push.followTags set to true * upstream repo had about 20k tags (despite only 55k commits) * his repo had an additional 2.5k tags, but none of these were in the history of the branches he was pushing and thus would not be included in any pushes. Digging in, almost all the time was CPU-bound and spent in add_missing_tags()[2]. If I'm reading the code correctly, it appears that function loops over each tag, calling in_merge_bases_many() once per tag. Thus, for his case, we were potentially walking all of history of the main branch 2.5k times. That seemed rather suboptimal. Before attempting to optimize, I decided to try out the commit-graph with a version of git from pu. While I expected a speed-up, I was a bit suprised that it was a factor of over 100; dropping the time for local dry-run push[2] to sub-second. A quick look suggests that commit-graph doesn't fix the fact that we call in_merge_bases_many() N times from add_missing_tags() and thus likely need to do N merge base computations, it just makes each of the N much faster. So, perhaps there's still another scaling issue we'll eventually need to address, but for now, I'm pretty excited about commit-graph. (And in the mean time I gave the user a one-liner to nuke his local-only tags that I suspect he doesn't need.) Thanks, Elijah [1] lerna seems to scale horribly, especially when you suddenly transition dozens of web developers and even more independent repositories into a single large monorepo. Usage of lerna was thankfully ripped out at some point, but the crazy number of historical tags remain. Also, this user did a bunch of the filter-branch'ing to suck extra repos into the monorepo, likely involved somehow in the many extra tags he had. [2] In fact, I still had timings of over a minute when adjusting the command to: git push --follow-tags --dry-run /PATH/TO/LOCAL-MIRROR $BRANCH
Re: [PATCH 00/19] Bring more repository handles into our code base
On Wed, Oct 17, 2018 at 5:41 AM Derrick Stolee wrote: > > On 10/16/2018 7:35 PM, Stefan Beller wrote: > > > > This series takes another approach as it doesn't change the signature > > of > > functions, but introduces new functions that can deal with arbitrary > > repositories, keeping the old function signature around using a > > shallow wrapper. > I think this is a good direction, and the changes look good to me. > > > Additionally each patch adds a semantic patch, that would port from > > the old to > > the new function. These semantic patches are all applied in the very > > last patch, > > but we could omit applying the last patch if it causes too many merge > > conflicts > > and trickl in the semantic patches over time when there are no merge > > conflicts. > > The semantic patches are a good idea. At some point in the future, we > can submit a series that applies the patches to any leftover calls and > removes the old callers. We can hopefully rely on review (and the > semantic patches warning that there is work to do) to prevent new > callers from being introduced in future topics. That doesn't count > topics that come around while this one isn't merged down. For those topics still in flight, I added re-defines, e.g. #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS #define get_merge_bases(r1, r2) repo_get_merge_bases(the_repository, r1, r2) #endif so the base function still keeps working, and we can cleanup multiple times, until eventually, we can get rid of the base function. > I had one high-level question: How are we testing that these "arbitrary > repository" changes are safe? I did the bare minimum in conversions in this series, such that the submodule code tests successfully. So if we'd revert some parts, the submodule tests would break. > I just remember the issue we had with the > commit-graph code relying on arbitrary repositories, but then adding a > reference to the replace objects broke the code (because replace-objects > wasn't using arbitrary repos correctly, but the commit-graph was tested > with arbitrary repositories using "test-tool repository"). It would be > nice to introduce more method calls in t/helper/test-repository.c that > help us know these are safe conversions. Or instead we could accelerate the long term plan of removing a hard coded the_repository and have each cmd builtin take an additional repository pointer from the init code, such that we'd bring all of Git to work on arbitrary repositories. Then the standard test suite should be okay, as there is no special case for the_repository any more. > Otherwise, we are essentially > waiting until we try to take submodule things in-process and find out > what breaks. As we discovered with the refstore, we can't just ensure > that all references to the_repository are removed. Yes, that is correct. We had a similar case with partial clone, as laid out in the cover letter for the RFC. I'll explore both the test tool approach as well as repository-fication of the code base. Thanks, Stefan
Re: [PATCH v4] branch: introduce --show-current display option
On 10/17/18 11:39 AM, Rafael Ascensão wrote: > On Fri, Oct 12, 2018 at 03:33:21PM +0200, Daniels Umanovskis wrote: >> Intended both for scripting and interactive/informative use. >> Unlike git branch --list, no filtering is needed to just get the >> branch name. > > Are we going forward with advertising this as a scriptable alternative? That's probably up to the maintainers, but I would not explicitly point it out as a script command, so my patch doesn't mention scripting use in the documentation for it. In reality it's useful for "soft scripting" like setting the shell $PS1, which doesn't require API stability guarantees the way proper scripts do.
[PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet
From: Ben Peart
When git reset is run with the --quiet flag, don't bother finding any
additional unstaged changes as they won't be output anyway. This speeds up
the git reset command by avoiding having to lstat() every file looking for
changes that aren't going to be reported anyway.
The savings can be significant. In a repo with 200K files "git reset"
drops from 7.16 seconds to 0.32 seconds for a savings of 96%.
Signed-off-by: Ben Peart
---
Documentation/git-reset.txt | 4 +++-
builtin/reset.c | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 1d697d9962..8610309b55 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -95,7 +95,9 @@ OPTIONS
-q::
--quiet::
- Be quiet, only report errors.
+ Be quiet, only report errors. Can optimize the performance of reset
+ by avoiding scaning all files in the repo looking for additional
+ unstaged changes.
EXAMPLES
diff --git a/builtin/reset.c b/builtin/reset.c
index 11cd0dcb8c..0822798616 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -376,7 +376,7 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
if (read_from_tree(, , intent_to_add))
return 1;
if (get_git_work_tree())
- refresh_index(_index, flags, NULL, NULL,
+ refresh_index(_index, flags, quiet ?
: NULL, NULL,
_("Unstaged changes after
reset:"));
} else {
int err = reset_index(, reset_type, quiet);
--
2.18.0.windows.1
[PATCH v1 0/2] speed up git reset
From: Ben Peart The reset (mixed) command unstages the specified file(s) and then shows you the remaining unstaged changes. This can make the command slow on larger repos because at the end it calls refresh_index() which has a single thread that loops through all the entries calling lstat() for every file. If the user passes the --quiet switch, reset doesn�t display the remaining unstaged changes but it still does all the work to find them, it just doesn�t print them out so passing "--quiet" doesn�t help performance. This patch series will: 1) change the behavior of "git reset --quiet" so that it no longer computes the remaining unstaged changes. 2) add a new config setting so that "--quiet" can be configured as the default so that the default performance of "git reset" is improved. The performance benefit of this can be significant. In a repo with 200K files "git reset foo" performance drops from 7.16 seconds to 0.32 seconds for a savings of 96%. Even with the small git repo, reset times drop from 0.191 seconds to 0.043 seconds for a savings of 77%. Base Ref: master Web-Diff: https://github.com/benpeart/git/commit/2295a310d0 Checkout: git fetch https://github.com/benpeart/git reset-refresh-index-v1 && git checkout 2295a310d0 Ben Peart (2): reset: don't compute unstaged changes after reset when --quiet reset: add new reset.quietDefault config setting Documentation/config.txt| 6 ++ Documentation/git-reset.txt | 4 +++- builtin/reset.c | 3 ++- 3 files changed, 11 insertions(+), 2 deletions(-) base-commit: a4b8ab5363a32f283a61ef3a962853556d136c0e -- 2.18.0.windows.1
[PATCH v1 2/2] reset: add new reset.quietDefault config setting
From: Ben Peart
Add a reset.quietDefault config setting that sets the default value of the
--quiet flag when running the reset command. This enables users to change
the default behavior to take advantage of the performance advantages of
avoiding the scan for unstaged changes after reset. Defaults to false.
Signed-off-by: Ben Peart
---
Documentation/config.txt | 6 ++
builtin/reset.c | 1 +
2 files changed, 7 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index f6f4c21a54..a5cf4c019b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2728,6 +2728,12 @@ rerere.enabled::
`$GIT_DIR`, e.g. if "rerere" was previously used in the
repository.
+reset.quietDefault::
+ Sets the default value of the "quiet" option for the reset command.
+ Choosing "quiet" can optimize the performance of the reset command
+ by avoiding the scan of all files in the repo looking for additional
+ unstaged changes. Defaults to false.
+
include::sendemail-config.txt[]
sequence.editor::
diff --git a/builtin/reset.c b/builtin/reset.c
index 0822798616..7d151d48a0 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -306,6 +306,7 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
};
git_config(git_reset_config, NULL);
+ git_config_get_bool("reset.quietDefault", );
argc = parse_options(argc, argv, prefix, options, git_reset_usage,
PARSE_OPT_KEEP_DASHDASH);
--
2.18.0.windows.1
For your photos 20
We provide photoshop services to some of the companies from around the world. Some online stores use our services for retouching portraits, jewelry, apparels, furnitures etc. Here are the details of what we provide: Clipping path for photos Deep etching for photos Image masking for photos Portrait retouching for photos Please reply back for further info. We can provide testing for your photos if needed. Thanks, Jenny
For your photos 20
We provide photoshop services to some of the companies from around the world. Some online stores use our services for retouching portraits, jewelry, apparels, furnitures etc. Here are the details of what we provide: Clipping path for photos Deep etching for photos Image masking for photos Portrait retouching for photos Please reply back for further info. We can provide testing for your photos if needed. Thanks, Jenny
Re: [PATCH v2 10/13] Add a base implementation of SHA-256 support
On Mon, Oct 15, 2018 at 02:18:57AM +, brian m. carlson wrote:
> diff --git a/sha256/block/sha256.c b/sha256/block/sha256.c
> new file mode 100644
> index 00..18350c161a
> --- /dev/null
> +++ b/sha256/block/sha256.c
> @@ -0,0 +1,180 @@
> +#include "git-compat-util.h"
> +#include "./sha256.h"
> +
> +#define BLKSIZE blk_SHA256_BLKSIZE
> +
> +void blk_SHA256_Init(blk_SHA256_CTX *ctx)
> +{
> + ctx->offset = 0;
> + ctx->length = 0;
> + ctx->state[0] = 0x6A09E667UL;
> + ctx->state[1] = 0xBB67AE85UL;
> + ctx->state[2] = 0x3C6EF372UL;
> + ctx->state[3] = 0xA54FF53AUL;
> + ctx->state[4] = 0x510E527FUL;
> + ctx->state[5] = 0x9B05688CUL;
> + ctx->state[6] = 0x1F83D9ABUL;
> + ctx->state[7] = 0x5BE0CD19UL;
> +}
> +
> +static inline uint32_t ror(uint32_t x, unsigned n)
> +{
> + return (x >> n) | (x << (32 - n));
> +}
> +
> +#define Ch(x,y,z) (z ^ (x & (y ^ z)))
> +#define Maj(x,y,z) (((x | y) & z) | (x & y))
> +#define S(x, n) ror((x),(n))
> +#define R(x, n) ((x)>>(n))
> +#define Sigma0(x) (S(x, 2) ^ S(x, 13) ^ S(x, 22))
> +#define Sigma1(x) (S(x, 6) ^ S(x, 11) ^ S(x, 25))
> +#define Gamma0(x) (S(x, 7) ^ S(x, 18) ^ R(x, 3))
> +#define Gamma1(x) (S(x, 17) ^ S(x, 19) ^ R(x, 10))
[...]
> +#define RND(a,b,c,d,e,f,g,h,i,ki)\
> + t0 = h + Sigma1(e) + Ch(e, f, g) + ki + W[i]; \
> + t1 = Sigma0(a) + Maj(a, b, c); \
> + d += t0;\
> + h = t0 + t1;
> +
> + RND(S[0],S[1],S[2],S[3],S[4],S[5],S[6],S[7],0,0x428a2f98);
[...]
> +#undef RND
> +
> + for (i = 0; i < 8; i++) {
> + ctx->state[i] = ctx->state[i] + S[i];
> + }
> +}
> +
> +#define MIN(x, y) ((x) < (y) ? (x) : (y))
On macOS there is a MIN macro already defined in the system headers,
resulting in the following error:
CC sha256/block/sha256.o
sha256/block/sha256.c:133:9: error: 'MIN' macro redefined
[-Werror,-Wmacro-redefined]
#define MIN(x, y) ((x) < (y) ? (x) : (y))
^
/usr/include/sys/param.h:215:9: note: previous definition is here
#define MIN(a,b) (((a)<(b))?(a):(b))
^
1 error generated.
make: *** [sha256/block/sha256.o] Error 1
A simple "#undef MIN" solves this issue. However, I wonder whether we
should #undef the other #define directives as well, just to be sure
(and perhaps overly cautious).
> +void blk_SHA256_Update(blk_SHA256_CTX *ctx, const void *data, size_t len)
> +{
> + const unsigned char *in = data;
> + size_t n;
> + ctx->length += len;
> + while (len > 0) {
> + if (!ctx->offset && len >= BLKSIZE) {
> + blk_SHA256_Transform(ctx, in);
> + in += BLKSIZE;
> + len -= BLKSIZE;
> + } else {
> + n = MIN(len, (BLKSIZE - ctx->offset));
> + memcpy(ctx->buf + ctx->offset, in, n);
> + ctx->offset += n;
> + in += n;
> + len -= n;
> + if (ctx->offset == BLKSIZE) {
> + blk_SHA256_Transform(ctx, ctx->buf);
> + ctx->offset = 0;
> + }
> + }
> + }
> +}
> +
> +void blk_SHA256_Final(unsigned char *digest, blk_SHA256_CTX *ctx)
> +{
> + const unsigned trip = BLKSIZE - sizeof(ctx->length);
> + int i;
> +
> + ctx->length <<= 3;
> + ctx->buf[ctx->offset++] = 0x80;
> +
> + if (ctx->offset > trip) {
> + memset(ctx->buf + ctx->offset, 0, BLKSIZE - ctx->offset);
> + blk_SHA256_Transform(ctx, ctx->buf);
> + ctx->offset = 0;
> + }
> +
> + memset(ctx->buf + ctx->offset, 0, BLKSIZE - ctx->offset -
> sizeof(ctx->length));
> +
> + put_be64(ctx->buf + trip, ctx->length);
Some GCC versions (e.g. gcc-4.8 with -O2 -Wall -Werror) complain about
the above line:
CC sha256/block/sha256.o
sha256/block/sha256.c: In function ‘blk_SHA256_Final’:
sha256/block/sha256.c:174:2: error: dereferencing type-punned pointer will
break strict-aliasing rules [-Werror=strict-aliasing]
put_be64(ctx->buf + trip, ctx->length);
^
cc1: all warnings being treated as errors
make: *** [sha256/block/sha256.o] Error 1
Something like this makes it compile:
void *ptr = ctx->buf + trip;
put_be64(ptr, ctx->length);
However, it's not immediately obvious to me why the compiler
complains, or why that intermediate void* variable makes any
difference, but now it's not the time to put on my language lawyer
hat.
Perhaps an old compiler bug? Clang in general, newer GCC versions, or
gcc-4.8 with -Wall -Werror but without -O2 don't seem to be affected.
> + blk_SHA256_Transform(ctx, ctx->buf);
> +
> + /* copy output */
> + for (i = 0; i < 8; i++, digest += sizeof(uint32_t))
> + put_be32(digest, ctx->state[i]);
> +}
Re: [PATCH v2 06/13] Add a build definition for Azure DevOps
Hi Gábor,
On Tue, 16 Oct 2018, SZEDER Gábor wrote:
> On Mon, Oct 15, 2018 at 03:12:06AM -0700, Johannes Schindelin via
> GitGitGadget wrote:
> > diff --git a/azure-pipelines.yml b/azure-pipelines.yml
> > new file mode 100644
> > index 00..b5749121d2
> > --- /dev/null
> > +++ b/azure-pipelines.yml
> > @@ -0,0 +1,319 @@
> > +resources:
> > +- repo: self
> > + fetchDepth: 1
> > +
> > +phases:
> > +- phase: linux_clang
> > + displayName: linux-clang
> > + condition: succeeded()
> > + queue:
> > +name: Hosted Ubuntu 1604
> > + steps:
> > + - bash: |
> > + test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' ||
> > ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache
> > gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1
> > +
> > + sudo apt-get update &&
> > + sudo apt-get -y install git gcc make libssl-dev
> > libcurl4-openssl-dev libexpat-dev tcl tk gettext git-email zlib1g-dev
> > apache2-bin &&
> > +
> > + export CC=clang || exit 1
> > +
> > + ci/install-dependencies.sh
>
> I think you would want to 'exit 1' when this script fails.
> This applies to other build jobs (erm, phases?) below as well.
True.
FWIW the nomenclature is "build" or "job" or "build job" for the entire
run, from what I understand. The "phase" is the individual chunk that is
run in an individual agent, i.e. you can have a single job running test on
different OSes in separate phases.
> > + ci/run-build-and-tests.sh || {
> > + ci/print-test-failures.sh
> > + exit 1
> > + }
> > +
> > + test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || sudo umount
> > "$HOME/test-cache" || exit 1
> > +displayName: 'ci/run-build-and-tests.sh'
> > +env:
> > + GITFILESHAREPWD: $(gitfileshare.pwd)
> > + - task: PublishTestResults@2
> > +displayName: 'Publish Test Results **/TEST-*.xml'
> > +inputs:
> > + mergeTestResults: true
> > + testRunTitle: 'linux-clang'
> > + platform: Linux
> > + publishRunAttachments: false
> > +condition: succeededOrFailed()
> > +
> > +- phase: linux_gcc
> > + displayName: linux-gcc
> > + condition: succeeded()
> > + queue:
> > +name: Hosted Ubuntu 1604
> > + steps:
> > + - bash: |
> > + test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' ||
> > ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache
> > gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1
> > +
> > + sudo apt-get update &&
> > + sudo apt-get -y install git gcc make libssl-dev
> > libcurl4-openssl-dev libexpat-dev tcl tk gettext git-email zlib1g-dev
> > apache2-bin || exit 1
> > +
>
> On Travis CI the Linux GCC build job uses gcc-8 instead of whatever
> the default is in that old-ish Ubuntu LTS; see 37fa4b3c78 (travis-ci:
> run gcc-8 on linux-gcc jobs, 2018-05-19).
I'll add those dependencies explicitly. It does seem, however, from a
cursory look at the log, that gcc-8 should not even be picked up, as it is
set via the environment variable `CC` (which, as you point out in
below-referenced thread, is not respected):
[...]
2018-10-16T10:00:36.0177072Z ++ '[' linux-gcc = linux-gcc ']'
2018-10-16T10:00:36.0177380Z ++ export CC=gcc-8
2018-10-16T10:00:36.0177630Z ++ CC=gcc-8
2018-10-16T10:00:36.0177917Z ++ case "$jobname"
[...]
(see https://dev.azure.com/git/git/_build/results?buildId=192=logs)
> > + ci/install-dependencies.sh
> > + ci/run-build-and-tests.sh || {
> > + ci/print-test-failures.sh
> > + exit 1
> > + }
> > +
> > + test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || sudo umount
> > "$HOME/test-cache" || exit 1
> > +displayName: 'ci/run-build-and-tests.sh'
> > +env:
> > + GITFILESHAREPWD: $(gitfileshare.pwd)
> > + - task: PublishTestResults@2
> > +displayName: 'Publish Test Results **/TEST-*.xml'
> > +inputs:
> > + mergeTestResults: true
> > + testRunTitle: 'linux-gcc'
> > + platform: Linux
> > + publishRunAttachments: false
> > +condition: succeededOrFailed()
> > +
> > +- phase: osx_clang
> > + displayName: osx-clang
> > + condition: succeeded()
> > + queue:
> > +name: Hosted macOS
> > + steps:
> > + - bash: |
> > + test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' ||
> > ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache
> > gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1
> > +
> > + export CC=clang
> > +
> > + ci/install-dependencies.sh
> > + ci/run-build-and-tests.sh || {
> > + ci/print-test-failures.sh
> > + exit 1
> > + }
> > +
> > + test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || umount
> > "$HOME/test-cache" || exit 1
> > +displayName: 'ci/run-build-and-tests.sh'
> > +env:
> > + GITFILESHAREPWD: $(gitfileshare.pwd)
> > + - task: PublishTestResults@2
> > +displayName: 'Publish Test Results **/TEST-*.xml'
> > +inputs:
> > +
[RFC] cherry-pick notes to find out cherry-picks from the origin
Hello, Junio, Jeff. A while ago, I proposed changes to name-rev and describe so that they can identify the commits cherry-picked from the one which is being shown. https://public-inbox.org/git/20180726153714.gx1934...@devbig577.frc2.facebook.com/T/ While the use-cases - e.g. tracking down which release / stable branches a given commit ended up in - weren't controversial, it was suggested that it'd make more sense to use notes to link cherry-picks instead of building the feature into name-rev. The patch appended to this message implements most of it (sans tests and documentation). It's composed of the following two parts. * A new built-in command note-cherry-picks, which walks the specified commits and if they're marked with the cherry-pick trailer, adds the backlink to the origin commit using Cherry-picked-to tag in a cherry-picks note. * When formatting a cherry-picks note for display, nested cherry-picks are followed from each Cherry-picked-to tag and printed out with matching indentations. Combined with name-rev --stdin, it can produce outputs like the following. commit 82cddd79f962de0bb1e7cdd95d48b4865816 (branch2) Author: Tejun Heo Date: Wed Jul 25 21:31:35 2018 -0700 commit 4 (cherry picked from commit 10f7ce0a0e524279f022b48460c088a108b45d54 (master~1)) (cherry picked from commit d433e3b4d5a19b3d29e2c8349fe88ceade5f6190 (branch1)) commit d433e3b4d5a19b3d29e2c8349fe88ceade5f6190 (branch1) Author: Tejun Heo Date: Wed Jul 25 21:31:35 2018 -0700 commit 4 (cherry picked from commit 10f7ce0a0e524279f022b48460c088a108b45d54 (master~1)) Notes (cherry-picks): Cherry-picked-to: 82cddd79f962de0bb1e7cdd95d48b4865816 (branch2) commit 10f7ce0a0e524279f022b48460c088a108b45d54 (master~1) Author: Tejun Heo Date: Wed Jul 25 21:31:35 2018 -0700 commit 4 Notes (cherry-picks): Cherry-picked-to: d433e3b4d5a19b3d29e2c8349fe88ceade5f6190 (branch1) Cherry-picked-to: 82cddd79f962de0bb1e7cdd95d48b4865816 (branch2) Cherry-picked-to: 2dd08fe869986c26bc1152a0bcec8c2fa48c50f7 (branch5) Cherry-picked-to: fa8b79edc5dfff21753c2ccfc1a1828336c4c070 (branch4~5) Cherry-picked-to: a1fb6024e3bda5549de1d15d8fa37e8c3a7eecbe (branch4~2) Cherry-picked-to: 58964342321a65e316ff47db33f7063743bc0de8 (branch4) Cherry-picked-to: 45e0d5f31c869dcc89b9737853e64489e2ad80b0 (branch4~1) Cherry-picked-to: 58a8d36b2532feb0a14b4fc2a50d587e64f38324 (branch3) Locally, the notes can be kept up-to-date with a trivial post-commit hook which invokes note-cherry-picks on the new commit; however, I'm having a bit of trouble figuring out a way to keep it up-to-date when multiple trees are involved. AFAICS, there are two options. 1. Ensuring that the notes are always generated on local commits and whenever new commits are received through fetch/pulls. 2. Ensuring that the notes are always generated on local commits and transported with push/pulls. 3. A hybrid approach - also generate notes on the receiving end and ensure that fetch/pulls receives the notes together (ie. similar to --tags option to git-fetch). #1 seems simpler and more robust to me. Unfortunately, I can't see a way to implement any of the three options with the existing hooks. For #1, there's no post-fetch hook. For #2 and #3, there doesn't seem to be a fool-proof way to make sure that the notes are transported together. Any suggestions would be greatly appreciated. Please let me know what you think. Thanks. --- Makefile|1 builtin.h |1 builtin/note-cherry-picks.c | 197 builtin/notes.c | 17 ++- git.c |1 notes.c | 95 + notes.h |7 + object.c|4 object.h|6 + 9 files changed, 320 insertions(+), 9 deletions(-) diff --git a/Makefile b/Makefile index 5c8307b7c..fb0ff3ce9 100644 --- a/Makefile +++ b/Makefile @@ -1073,6 +1073,7 @@ BUILTIN_OBJS += builtin/multi-pack-index.o BUILTIN_OBJS += builtin/mv.o BUILTIN_OBJS += builtin/name-rev.o BUILTIN_OBJS += builtin/notes.o +BUILTIN_OBJS += builtin/note-cherry-picks.o BUILTIN_OBJS += builtin/pack-objects.o BUILTIN_OBJS += builtin/pack-redundant.o BUILTIN_OBJS += builtin/pack-refs.o diff --git a/builtin.h b/builtin.h index 962f0489a..d9d019abc 100644 --- a/builtin.h +++ b/builtin.h @@ -195,6 +195,7 @@ extern int cmd_multi_pack_index(int argc, const char **argv, const char *prefix) extern int cmd_mv(int argc, const char **argv, const char *prefix); extern int cmd_name_rev(int argc, const char **argv, const char *prefix); extern int cmd_notes(int argc, const char **argv, const char *prefix); +extern int cmd_note_cherry_picks(int argc, const char **argv, const char
Re: [PATCH v2 13/13] commit-graph: specify OID version for SHA-256
On Wed, Oct 17, 2018 at 12:44 AM brian m. carlson
wrote:
> > > >> static uint8_t oid_version(void)
> > > >> {
> > > >> - return 1;
> > > >> + switch (hash_algo_by_ptr(the_hash_algo)) {
> > > >> + case GIT_HASH_SHA1:
> > > >> + return 1;
> > > >> + case GIT_HASH_SHA256:
> > > >> + return 2;
> > > > Should we just increase this field to uint32_t and store format_id
> > > > instead? That will keep oid version unique in all data formats.
> > > Both the commit-graph and multi-pack-index store a single byte for the
> > > hash version, so that ship has sailed (without incrementing the full
> > > file version number in each format).
> >
> > And it's probably premature to add the oid version field when multiple
> > hash support has not been fully realized. Now we have different ways
> > of storing hash id and need separate mappings.
>
> Honestly, anything in the .git directory that is not the v3 pack indexes
> or the loose object file should be in exactly one hash algorithm. We
> could simply just leave this value at 1 all the time and ignore the
> field, since we already know what algorithm it will use.
In this particular case, I agree, but not as a general principle. It's
nice to have independence for fsck-like tools. I don't know if we have
a tool that simply validates commit-graph file format (and not trying
to access any real object). But for such a tool, I guess we can just
pass the hash algorithm from command line. The user would have to
guess a bit.
--
Duy
Re: On overriding make variables from the environment...
On Tue, Oct 16, 2018 at 03:40:01PM -0700, Jonathan Nieder wrote: > SZEDER Gábor wrote: > > On Tue, Oct 16, 2018 at 02:54:56PM -0700, Jonathan Nieder wrote: > >> SZEDER Gábor wrote: > > >>> Our Makefile has lines like these: > >>> > >>> CFLAGS = -g -O2 -Wall > >>> CC = cc > >>> AR = ar > >>> SPATCH = spatch > [...] > >>> I'm not sure what to do. I'm fine with updating our 'ci/' scripts to > >>> explicitly respect CC in the environment (either by running 'make > >>> CC=$CC' or by writing $CC into 'config.mak'). Or I could update our > >>> Makefile to use '?=' for specific variables, but: > >> > >> That's a good question. I don't have a strong opinion myself, so I > >> tend to trust larger projects like Linux to have thought this through > >> more, and they use 'CC = cc' as well. > > > > I don't think Linux is a good example to follow in this case, see e.g. > > 6d62c983f7 (Makefile: Change the default compiler from "gcc" to "cc", > > 2011-12-20) (in short: Git is supposed to be buildable with compilers > > other than GCC as well, while Linux not really, so they can hardcode > > 'CC = gcc'). > > Nowadays Linux builds with clang as well. People also have other > reasons to override the CC setting (cross-compiling, etc) and to > override other settings like CFLAGS. > > > Also, the projects I have on hand usually have 'CC = gcc' hardcoded in > > their Makefiles, too, but those Makefiles were generated by their > > ./configure script (which in turn by ./autogen.sh...), and those tend > > to write CC from the environment into the generated Makefiles. > > Yes, I think that's what makes travis's setup normally work. It makes > sense to me since ./configure is expected to have more implicit or > automatic behavior. For "make", I kind of like that it doesn't depend > on environment variables that I am not thinking about when debugging a > reported build problem. > > When building Git without autoconf, the corresponding place for > settings is config.mak. Would it make sense for the ci scripts to > write a config.mak file? A first I though it doesn't, but it turns out it acually does. 'ci/run-build-and-tests.sh' basically runs: make make test I naively put a 'CC=$CC' argument at the end of the first command, thinking it should Just Work... but then that second 'make test' got all clever on me, said "* new build flags", and then proceeded to rebuild everything with the default 'cc'. (With the additional complication, that on Travis CI we actually run 'make --quiet test', which rebuilds everything, well, quietly... so the rebuild itself is not even visible in the build logs.) So, then it's either 'config.mak', or passing a 'CC=$CC' argument to _all_ make commands, including those that are not supposed to build anything, but only run the tests. I find the latter aesthetically not particularly pleasing.
Dear
Dear,i am lisa jaster,it would be great to know you,i have a very important and confidential matter that i want to discuss with you,reply me back for more discus. Regards, Lisa Jaster.
Re: [RFC] revision: Add --sticky-default option
On Wed, 17 Oct 2018 at 11:12, Jeff King wrote:
> On Tue, Oct 16, 2018 at 11:24:38PM +0200, Andreas Gruenbacher wrote:
> > here's a long-overdue update of my proposal from August 29:
> >
> > [RFC] revision: Don't let ^ cancel out the default
> >
> > Does this look more acceptable that my first shot?
>
> I think it's going in the right direction.
>
> The name "--sticky-default" did not immediately make clear to me what it
> does. Is there some name that would be more obvious?
It's the best I could think of. Better ideas, anyone?
> > Some commands like 'log' default to HEAD if no other revisions are
> > specified on the command line or otherwise. Currently, excludes
> > (^) cancel out that default, so when a command only excludes
> > revisions (e.g., 'git log ^origin/master'), it will produce no output.
> >
> > With the --sticky-default option, the default becomes more "sticky" and
> > is no longer canceled out by excludes.
> >
> > This is useful in wrappers that exclude certain revisions: for example,
> > a simple alias l='git log --sticky-default ^origin/master' will show the
> > revisions between origin/master and HEAD when invoked without arguments,
> > and 'l foo' will show the revisions between origin/master and foo.
>
> Your explanation makes sense.
>
> > ---
> > revision.c | 31 +++
> > revision.h | 1 +
> > t/t4202-log.sh | 6 ++
>
> We'd probably want an update to Documentation/rev-list-options.txt (I
> notice that "--default" is not covered there; that might be worth
> fixing, too)>
Ok.
> > +static int has_revisions(struct rev_info *revs)
> > +{
> > + struct rev_cmdline_info *info = >cmdline;
> > +
> > + return info->nr != 0;
> > +}
>
> So this function is going to replace this flag:
>
> > @@ -2401,8 +2423,6 @@ int setup_revisions(int argc, const char **argv,
> > struct rev_info *revs, struct s
> > argv_array_pushv(_data, argv + i);
> > break;
> > }
> > - else
> > - got_rev_arg = 1;
> > }
>
> Are we sure that every that hits that "else" is going to trigger
> info->nr (and vice versa)?
>
> If I say "--tags", I think we may end up with entries in revs->cmdline,
> but would not have set got_rev_arg. That's captured separately in
> revs->rev_input_given. But your cancel_default logic:
>
> > @@ -2431,7 +2451,10 @@ int setup_revisions(int argc, const char **argv,
> > struct rev_info *revs, struct s
> > opt->tweak(revs, opt);
> > if (revs->show_merge)
> > prepare_show_merge(revs);
> > - if (revs->def && !revs->pending.nr && !revs->rev_input_given &&
> > !got_rev_arg) {
> > + cancel_default = revs->sticky_default ?
> > + has_interesting_revisions(revs) :
> > + has_revisions(revs);
> > + if (revs->def && !revs->rev_input_given && !cancel_default) {
>
> doesn't handle that. So if I do:
>
> git rev-list --count --sticky-default --default HEAD --not --tags
>
> I should see everything in HEAD that's not tagged. But we don't even
> look at cancel_default, because !revs->rev_input_given is not true.
>
> I think you could solve that by making the logic more like:
>
> if (revs->sticky_default)
> cancel_default = has_interesting_revisions();
> else
> cancel_default = !revs->rev_input_given && !got_rev_arg;
>
> which leaves the non-sticky case exactly as it is today.
Right, I've reworked that.
> > diff --git a/revision.h b/revision.h
> > index 2b30ac270..570fa1a6d 100644
> > --- a/revision.h
> > +++ b/revision.h
> > @@ -92,6 +92,7 @@ struct rev_info {
> >
> > unsigned int early_output;
> >
> > + unsigned intsticky_default:1;
> > unsigned intignore_missing:1,
> > ignore_missing_links:1;
>
> Maybe it would make sense to put this next to "const char *def"?
>
> The bitfield would not be as efficient, but I don't think we care about
> packing rev_info tightly.
Ok.
> > diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> > index 153a50615..9517a65da 100755
> > --- a/t/t4202-log.sh
> > +++ b/t/t4202-log.sh
> > @@ -213,6 +213,12 @@ test_expect_success 'git show leaves list of
> > commits as given' '
> > test_cmp expect actual
> > '
> >
> > +printf "sixth\nfifth\n" > expect
> > +test_expect_success '--sticky-default ^' '
> > + git log --pretty="tformat:%s" --sticky-default ^HEAD~2 > actual &&
> > + test_cmp expect actual
> > +'
>
> Yuck, t4202 is a mix of older and newer styles. I'm OK with this as-is
> because you've matched the surrounding code, but these days I'd probably
> write:
>
> test_expect_success '--sticky-default ^' '
> {
> echo sixth
> echo fifth
> } >expect &&
> git log --format=%s --sticky-default ^HEAD~2 >actual &&
> test_cmp expect actual
> '
I don't really want to get hung up on such details. test_write_lines
doesn't
[RFC v2] revision: Add --sticky-default option
Some commands like 'log' default to HEAD if no other revisions are
specified on the command line or otherwise. Currently, excludes
(^) cancel out that default, so when a command only excludes
revisions (e.g., 'git log ^origin/master'), it will produce no output.
With the --sticky-default option, the default becomes more "sticky" and
is no longer canceled out by excludes.
This is useful in wrappers that exclude certain revisions: for example,
a simple alias l='git log --sticky-default ^origin/master' will show the
revisions between origin/master and HEAD when invoked without arguments,
and 'l foo' will show the revisions between origin/master and foo.
Signed-off-by: Andreas Gruenbacher
---
Documentation/rev-list-options.txt | 10 ++
revision.c | 21 -
revision.h | 1 +
t/t4202-log.sh | 6 ++
4 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index 7b273635d..46fab2b42 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -210,6 +210,16 @@ endif::git-rev-list[]
seen, stop reading commits and start reading paths to limit the
result.
+--default=::
+ Instead of `HEAD`, use '' when no revisions are specified.
+
+--sticky-default:
+ By default, excludes ('^') override the default revision (i.e.
+ `HEAD` or the revision specified with `--default`). This option causes
+ excludes to no longer override the default revision, so commands like
+ `git log --sticky-default ^origin/master` will continue to operate on
+ the default revision as long as no other revisions are specified.
+
ifdef::git-rev-list[]
--quiet::
Don't print anything to standard output. This form
diff --git a/revision.c b/revision.c
index e18bd530e..e61f28b92 100644
--- a/revision.c
+++ b/revision.c
@@ -1159,6 +1159,18 @@ static void add_rev_cmdline(struct rev_info *revs,
info->nr++;
}
+static int has_interesting_revisions(struct rev_info *revs)
+{
+ struct rev_cmdline_info *info = >cmdline;
+ unsigned int n;
+
+ for (n = 0; n < info->nr; n++) {
+ if (!(info->rev[n].flags & UNINTERESTING))
+ return 1;
+ }
+ return 0;
+}
+
static void add_rev_cmdline_list(struct rev_info *revs,
struct commit_list *commit_list,
int whence,
@@ -2132,6 +2144,8 @@ static int handle_revision_opt(struct rev_info *revs, int
argc, const char **arg
} else if (!strcmp(arg, "--children")) {
revs->children.name = "children";
revs->limited = 1;
+ } else if (!strcmp(arg, "--sticky-default")) {
+ revs->sticky_default = 1;
} else if (!strcmp(arg, "--ignore-missing")) {
revs->ignore_missing = 1;
} else if (!strcmp(arg, "--exclude-promisor-objects")) {
@@ -2320,6 +2334,7 @@ static void NORETURN diagnose_missing_default(const char
*def)
int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct
setup_revision_opt *opt)
{
int i, flags, left, seen_dashdash, got_rev_arg = 0, revarg_opt;
+ int cancel_default;
struct argv_array prune_data = ARGV_ARRAY_INIT;
const char *submodule = NULL;
@@ -2431,7 +2446,11 @@ int setup_revisions(int argc, const char **argv, struct
rev_info *revs, struct s
opt->tweak(revs, opt);
if (revs->show_merge)
prepare_show_merge(revs);
- if (revs->def && !revs->pending.nr && !revs->rev_input_given &&
!got_rev_arg) {
+ if (revs->sticky_default)
+ cancel_default = has_interesting_revisions();
+ else
+ cancel_default = got_rev_arg;
+ if (revs->def && !revs->rev_input_given && !cancel_default) {
struct object_id oid;
struct object *object;
struct object_context oc;
diff --git a/revision.h b/revision.h
index 2b30ac270..6498ba263 100644
--- a/revision.h
+++ b/revision.h
@@ -73,6 +73,7 @@ struct rev_info {
/* Basic information */
const char *prefix;
const char *def;
+ unsigned int sticky_default:1;
struct pathspec prune_data;
/*
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 153a50615..5ac93f5ec 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -213,6 +213,12 @@ test_expect_success 'git show leaves list of
commits as given' '
test_cmp expect actual
'
+test_expect_success '--sticky-default ^' '
+ test_write_lines sixth fifth > expect &&
+ git log --pretty="tformat:%s" --sticky-default ^HEAD~2 > actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'setup case sensitivity tests' '
echo case >one &&
test_tick &&
--
2.17.2
Git Test Coverage Report (Wednesday, Oct 17)
In an effort to ensure new code is reasonably covered by the test suite,
we now have contrib/coverage-diff.sh to combine the gcov output from
'make coverage-test ; make coverage-report' with the output from 'git
diff A B' to discover _new_lines of code that are not covered. This
report ignores lines including "BUG(".
This report takes the output of these results after running on four
branches:
pu: 9b314678659b2d0a69e4ca4f77444ba66f158564
(build is broken)
jch: e6b0db6626cabc387dd5a8c60088421ce65e1686
(build is broken)
next: 3377e82b59874c0e626c00f29bf7526a27f29d65
master: a4b8ab5363a32f283a61ef3a962853556d136c0e
master@{1}: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf
I ran the test suite on each of these branches on an Ubuntu Linux VM,
and I'm missing some dependencies (like apache, svn, and perforce) so
not all tests are run
Thanks,
-Stolee
Uncovered Code in 'next' (3377e82) but not in 'master' (a4b8ab5)
blame.c
a470beea39 113) !strcmp(r->index->cache[-1 - pos]->name, path))
a470beea39 272) int pos = index_name_pos(r->index, path, len);
a470beea39 274) mode = r->index->cache[pos]->ce_mode;
builtin/am.c
2abf350385 1414) repo_init_revisions(the_repository, _info, NULL);
builtin/archive.c
e001fd3a50 builtin/archive.c 78) die(_("git archive: expected ACK/NAK,
got a flush packet"));
e001fd3a50 builtin/archive.c 80) if (starts_with(reader.line, "NACK "))
e001fd3a50 builtin/archive.c 81) die(_("git archive: NACK %s"),
reader.line + 5);
e001fd3a50 builtin/archive.c 82) if (starts_with(reader.line, "ERR "))
e001fd3a50 builtin/archive.c 83) die(_("remote error: %s"), reader.line
+ 4);
e001fd3a50 builtin/archive.c 84) die(_("git archive: protocol error"));
e001fd3a50 builtin/archive.c 89) die(_("git archive: expected a flush"));
fb19d32f05 builtin/archive.c 99) if (version != discover_version())
fb19d32f05 builtin/archive.c 100) die(_("git archive: received different
protocol versions in subsequent requests"));
builtin/rev-list.c
7c0fe330d5 builtin/rev-list.c 227) die("unexpected missing %s object '%s'",
7c0fe330d5 builtin/rev-list.c 228) type_name(obj->type),
oid_to_hex(>oid));
builtin/upload-archive.c
e001fd3a50 builtin/upload-archive.c 111) if (version == protocol_v0 ||
version == protocol_v1)
e001fd3a50 builtin/upload-archive.c 112) packet_write_fmt(1, "NACK
unable to spawn subprocess\n");
e001fd3a50 builtin/upload-archive.c 113) else if (version == protocol_v2)
e001fd3a50 builtin/upload-archive.c 114) error_clnt("unable to spawn
subprocess\n");
config.c
c780b9cfe8 2298) return val;
c780b9cfe8 2301) if (is_bool)
c780b9cfe8 2302) return val ? 0 : 1;
c780b9cfe8 2304) return val;
diff.c
b78ea5fc35 4128) add_external_diff_name(o->repo, , other, two);
help.c
26c7d06783 help.c 500) static int get_alias(const char *var,
const char *value, void *data)
26c7d06783 help.c 502) struct string_list *list = data;
26c7d06783 help.c 504) if (skip_prefix(var, "alias.", ))
26c7d06783 help.c 505) string_list_append(list, var)->util =
xstrdup(value);
26c7d06783 help.c 507) return 0;
26c7d06783 help.c 530) printf("\n%s\n", _("Command aliases"));
26c7d06783 help.c 531) ALLOC_ARRAY(aliases, alias_list.nr + 1);
26c7d06783 help.c 532) for (i = 0; i < alias_list.nr; i++) {
26c7d06783 help.c 533) aliases[i].name = alias_list.items[i].string;
26c7d06783 help.c 534) aliases[i].help = alias_list.items[i].util;
26c7d06783 help.c 535) aliases[i].category = 1;
26c7d06783 help.c 537) aliases[alias_list.nr].name = NULL;
26c7d06783 help.c 538) print_command_list(aliases, 1, longest);
26c7d06783 help.c 539) free(aliases);
http-backend.c
fb19d32f05 646) argv[1] = ".";
fb19d32f05 647) argv[2] = NULL;
list-objects-filter-options.c
bc5975d24f 55) if (errbuf) {
bc5975d24f 56) strbuf_addstr(
bc5975d24f 60) return 1;
cc0b05a4cc 86) if (errbuf)
list-objects-filter.c
list-objects.c
f447a499db 197) ctx->show_object(obj, base->buf, ctx->show_data);
oidset.c
8b2f8cbcb1 29) kh_del_oid(>set, pos);
8b2f8cbcb1 30) return 1;
preload-index.c
ae9af12287 73) struct progress_data *pd = p->progress;
ae9af12287 75) pthread_mutex_lock(>mutex);
ae9af12287 76) pd->n += last_nr - nr;
ae9af12287 77) display_progress(pd->progress, pd->n);
ae9af12287 78) pthread_mutex_unlock(>mutex);
ae9af12287 79) last_nr = nr;
ae9af12287 93) struct progress_data *pd = p->progress;
ae9af12287 95) pthread_mutex_lock(>mutex);
ae9af12287 96) display_progress(pd->progress, pd->n + last_nr);
ae9af12287 97) pthread_mutex_unlock(>mutex);
ae9af12287 128) pd.progress = start_delayed_progress(_("Refreshing
index"), index->cache_nr);
ae9af12287 129) pthread_mutex_init(, NULL);
ae9af12287 140) p->progress =
read-cache.c
ae9af12287 1490) progress = start_delayed_progress(_("Refresh index"),
ae9af12287
Re: [PATCH v2 02/13] sha1-file: provide functions to look up hash algorithms
On Mon, Oct 15, 2018 at 02:18:49AM +, brian m. carlson wrote:
> There are several ways we might refer to a hash algorithm: by name, such
> as in the config file; by format ID, such as in a pack; or internally,
> by a pointer to the hash_algos array. Provide functions to look up hash
> algorithms based on these various forms and return the internal constant
> used for them. If conversion to another form is necessary, this
> internal constant can be used to look up the proper data in the
> hash_algos array.
>
> Signed-off-by: brian m. carlson
> ---
> hash.h | 13 +
> sha1-file.c | 21 +
> 2 files changed, 34 insertions(+)
>
> diff --git a/hash.h b/hash.h
> index 7c8238bc2e..90f4344619 100644
> --- a/hash.h
> +++ b/hash.h
> @@ -98,4 +98,17 @@ struct git_hash_algo {
> };
> extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
>
> +/*
> + * Return a GIT_HASH_* constant based on the name. Returns GIT_HASH_UNKNOWN
> if
> + * the name doesn't match a known algorithm.
> + */
> +int hash_algo_by_name(const char *name);
> +/* Identical, except based on the format ID. */
> +int hash_algo_by_id(uint32_t format_id);
> +/* Identical, except for a pointer to struct git_hash_algo. */
> +inline int hash_algo_by_ptr(const struct git_hash_algo *p)
This has to be declared as static, otherwise the linker will error out
when building without optimization:
LINK git
libgit.a(commit-graph.o): In function `oid_version':
/home/szeder/src/git/commit-graph.c:48: undefined reference to
`hash_algo_by_ptr'
libgit.a(hex.o): In function `hash_to_hex':
/home/szeder/src/git/hex.c:123: undefined reference to `hash_algo_by_ptr'
libgit.a(hex.o): In function `oid_to_hex':
/home/szeder/src/git/hex.c:128: undefined reference to `hash_algo_by_ptr'
collect2: error: ld returned 1 exit status
Makefile:2055: recipe for target 'git' failed
make: *** [git] Error 1
> +{
> + return p - hash_algos;
> +}
> +
> #endif
> diff --git a/sha1-file.c b/sha1-file.c
> index e29825f259..3a75d515eb 100644
> --- a/sha1-file.c
> +++ b/sha1-file.c
> @@ -122,6 +122,27 @@ const char *empty_blob_oid_hex(void)
> return oid_to_hex_r(buf, the_hash_algo->empty_blob);
> }
>
> +int hash_algo_by_name(const char *name)
> +{
> + int i;
> + if (!name)
> + return GIT_HASH_UNKNOWN;
> + for (i = 1; i < GIT_HASH_NALGOS; i++)
> + if (!strcmp(name, hash_algos[i].name))
> + return i;
> + return GIT_HASH_UNKNOWN;
> +}
> +
> +int hash_algo_by_id(uint32_t format_id)
> +{
> + int i;
> + for (i = 1; i < GIT_HASH_NALGOS; i++)
> + if (format_id == hash_algos[i].format_id)
> + return i;
> + return GIT_HASH_UNKNOWN;
> +}
> +
> +
> /*
> * This is meant to hold a *small* number of objects that you would
> * want read_sha1_file() to be able to return, but yet you do not want
Re: [RFC] revision: Add --sticky-default option
On Wed, 17 Oct 2018, Jeff King wrote:
Yuck, t4202 is a mix of older and newer styles. I'm OK with this as-is
because you've matched the surrounding code, but these days I'd probably
write:
test_expect_success '--sticky-default ^' '
{
echo sixth
echo fifth
} >expect &&
git log --format=%s --sticky-default ^HEAD~2 >actual &&
test_cmp expect actual
'
How about test_write_lines? That is a little more readable to me than
the echos in a subshell. A patch was recently queued with a usage of
that function:
https://public-inbox.org/git/camfpvhk4a15gd-pt3w+4yjmpe6c7hyhje5n_uqozu8gsyye...@mail.gmail.com/T/#m9b5ade1551722938ac97b75af58fec195f246c01
- Matt
Re: [PATCH 0/1] Run GIT_TEST_COMMIT_GRAPH and GIT_TEST_MULTI_PACK_INDEX during CI
On 10/17/2018 9:00 AM, Derrick Stolee via GitGitGadget wrote: [1] https://git.visualstudio.com/git/_build?definitionId=4 Newlines are hard. Sorry for the formatting issues when translating from a PR description. Build definition that tests Git with different arrangements of GIT_TEST_* variables. Thanks, -Stolee
Re: [PATCH] test-tool: show tool list on error
On 10/17/2018 5:25 AM, Jeff King wrote: Before we switched to one big test-tool binary, if you forgot the name of a tool, you could use tab-completion in the shell to get a hint. But these days, all you get is: $ t/helper/test-tool approxidate fatal: There is no test named 'approxidate' and you're stuck reading the source code to find it. Let's print a list of the available tools in this case. Signed-off-by: Jeff King --- Not really user-facing, but this bugged me enough earlier to write the patch. ;) Some of the individual tools have nice help, too (try "t/helper/test-tool date", which shows the approxidate command I was looking for), but some of them could probably stand to improve their friendliness (try "t/helper/test-tool config"). I think it's fine for people to improve them over time if and when they get annoyed. This will improve contributors' lives! Thanks. Reviewed-by: Derrick Stolee
[PATCH 0/1] Run GIT_TEST_COMMIT_GRAPH and GIT_TEST_MULTI_PACK_INDEX during CI
Our CI scripts include a step to run the test suite with certain optional variables enabled. Now that all branches should build and have tests succeed with GIT_TEST_COMMIT_GRAPH and GIT_TEST_MULTI_PACK_INDEX enabled, add those variables to that stage. Note: the GIT_TEST_MULTI_PACK_INDEX variable has not merged all the way down, so will be ignored if this series is merged faster than that one. However, it is safe to make these changes orthogonally as all (known) test breaks with GIT_TEST_MULTI_PACK_INDEX=1 are fixed in the topic that introduces the variable. I also created a build definition on Azure Pipelines that runs the test suite with different subsets of the test variables, split by the following types: 1) Index options 2) Commit-graph 3) Multi-pack-index These builds are found at [1]. There are benefits to testing the variables all together but also separately. I didn't want to create new stages in the CI scripts to avoid consuming extra resources. This series is based on js/vsts-ci to avoid conflicts with that series, but is not necessarily a hard dependence. Thanks, -Stolee [1] https://git.visualstudio.com/git/_build?definitionId=4Build definition that tests Git with different arrangements of GIT_TEST_* variables. Derrick Stolee (1): ci: add optional test variables ci/run-build-and-tests.sh | 2 ++ 1 file changed, 2 insertions(+) base-commit: d82963f34cf6921ed29d1fc2d96b16acf9005159 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-49%2Fderrickstolee%2Fci-vars-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-49/derrickstolee/ci-vars-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/49 -- gitgitgadget
[PATCH 1/1] ci: add optional test variables
From: Derrick Stolee The commit-graph and multi-pack-index features introduce optional data structures that are not required for normal Git operations. It is important to run the normal test suite without them enabled, but it is helpful to also run the test suite using them. Our continuous integration scripts include a second test stage that runs with optional GIT_TEST_* variables enabled. Add the following two variables to that stage: GIT_TEST_COMMIT_GRAPH GIT_TEST_MULTI_PACK_INDEX This will slow down the operation, as we build a commit-graph file after every 'git commit' operation and build a multi-pack-index during every 'git repack' operation. However, it is important that future changes are compatible with these features. Signed-off-by: Derrick Stolee --- ci/run-build-and-tests.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh index e28ac2fb9a..db342bb6a8 100755 --- a/ci/run-build-and-tests.sh +++ b/ci/run-build-and-tests.sh @@ -15,6 +15,8 @@ then export GIT_TEST_FULL_IN_PACK_ARRAY=true export GIT_TEST_OE_SIZE=10 export GIT_TEST_OE_DELTA_SIZE=5 + export GIT_TEST_COMMIT_GRAPH=1 + export GIT_TEST_MULTI_PACK_INDEX=1 make --quiet test fi -- gitgitgadget
Re: [PATCH v2] builtin/branch.c: remove useless branch_get
> > branch_get sometimes returns current_branch, which can be NULL (e.g., if > > you're on a detached HEAD). Try: > > > > $ git branch HEAD > > fatal: no such branch 'HEAD' > > > > $ git branch '' > > fatal: no such branch '' > > > > However, it seems weird that we'd check those cases here (and provide > > such lousy messages). And indeed, dropping that and letting us > > eventually hit create_branch() gives a much better message: > > > > $ git branch HEAD > > fatal: 'HEAD' is not a valid branch name. > > > > $ git branch '' > > fatal: '' is not a valid branch name. > > This explanation is perfect, of course. ;) > > I still wondered if you had another motivation hinted at in your > original mail, though (some weirdness with running branch_get early). > It's OK if there isn't one, but I just want to make sure we capture all > of the details. > Yes, this explanation is perfect. ;) > Other than that question, the patch looks good to me. > > -Peff
Re: [PATCH 00/19] Bring more repository handles into our code base
On 10/16/2018 7:35 PM, Stefan Beller wrote: This series takes another approach as it doesn't change the signature of functions, but introduces new functions that can deal with arbitrary repositories, keeping the old function signature around using a shallow wrapper. I think this is a good direction, and the changes look good to me. Additionally each patch adds a semantic patch, that would port from the old to the new function. These semantic patches are all applied in the very last patch, but we could omit applying the last patch if it causes too many merge conflicts and trickl in the semantic patches over time when there are no merge conflicts. The semantic patches are a good idea. At some point in the future, we can submit a series that applies the patches to any leftover calls and removes the old callers. We can hopefully rely on review (and the semantic patches warning that there is work to do) to prevent new callers from being introduced in future topics. That doesn't count topics that come around while this one isn't merged down. I had one high-level question: How are we testing that these "arbitrary repository" changes are safe? I just remember the issue we had with the commit-graph code relying on arbitrary repositories, but then adding a reference to the replace objects broke the code (because replace-objects wasn't using arbitrary repos correctly, but the commit-graph was tested with arbitrary repositories using "test-tool repository"). It would be nice to introduce more method calls in t/helper/test-repository.c that help us know these are safe conversions. Otherwise, we are essentially waiting until we try to take submodule things in-process and find out what breaks. As we discovered with the refstore, we can't just ensure that all references to the_repository are removed. Thanks, -Stolee
Re: [PATCH v2 13/13] commit-graph: specify OID version for SHA-256
On 10/14/2018 10:19 PM, brian m. carlson wrote:
Since the commit-graph code wants to serialize the hash algorithm into
the data store, specify a version number for each supported algorithm.
Note that we don't use the values of the constants themselves, as they
are internal and could change in the future.
Signed-off-by: brian m. carlson
---
commit-graph.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/commit-graph.c b/commit-graph.c
index 7a28fbb03f..e587c21bb6 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -45,7 +45,14 @@ char *get_commit_graph_filename(const char *obj_dir)
static uint8_t oid_version(void)
{
- return 1;
+ switch (hash_algo_by_ptr(the_hash_algo)) {
hash_algo_by_ptr is specified as an inline function in hash.h, so this
leads to a linking error in jch and pu right now.
I think the fix is simply to add '#include "hash.h"' to the list of
includes.
Thanks,
-Stolee
Re: [PATCH] upload-pack: clear flags before each v2 request
Hello, I can confirm that shallow clones are no longer failing. -- Arturas Moskvinas On Wed, Oct 17, 2018 at 12:58 AM Jonathan Tan wrote: > > Suppose a server has the following commit graph: > > A B > \ / >O > > We create a client by cloning A from the server with depth 1, and add > many commits to it (so that future fetches span multiple requests due to > lengthy negotiation). If it then fetches B using protocol v2, the fetch > spanning multiple requests, the resulting packfile does not contain O > even though the client did report that A is shallow. > > This is because upload_pack_v2() can be called multiple times while > processing the same session. During the 2nd and all subsequent > invocations, some object flags remain from the previous invocations. In > particular, CLIENT_SHALLOW remains, preventing process_shallow() from > adding client-reported shallows to the "shallows" array, and hence > pack-objects not knowing about these client-reported shallows. > > Therefore, teach upload_pack_v2() to clear object flags at the start of > each invocation. > > (One alternative is to reduce or eliminate usage of object flags in > protocol v2, but that doesn't seem feasible because almost all 8 flags > are used pervasively in v2 code.) > > Signed-off-by: Jonathan Tan > --- > This was noticed by Arturas Moskvinas in [1]. The > reproduction steps given were to repeat a shallow fetch twice in > succession, but I found it easier to write a more understandable test if > I made the 2nd fetch an ordinary fetch. In any case, I also reran the > original reproduction steps, and the fetch completes without error. > > This patch doesn't cover the negotiation issue that I mentioned in my > previous reply [2]. > > [1] > https://public-inbox.org/git/CAGY-PBgsG-T3JY=awszwgmpfx+jdx-a1fcv0s6vr067bsqg...@mail.gmail.com/ > [2] > https://public-inbox.org/git/20181013004356.257709-1-jonathanta...@google.com/ > --- > t/t5702-protocol-v2.sh | 25 + > upload-pack.c | 5 + > 2 files changed, 30 insertions(+) > > diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh > index 88a886975d..70b88385ba 100755 > --- a/t/t5702-protocol-v2.sh > +++ b/t/t5702-protocol-v2.sh > @@ -429,6 +429,31 @@ test_expect_success 'fetch supports include-tag and tag > following' ' > git -C client cat-file -e $(git -C client rev-parse annotated_tag) > ' > > +test_expect_success 'upload-pack respects client shallows' ' > + rm -rf server client trace && > + > + git init server && > + test_commit -C server base && > + test_commit -C server client_has && > + > + git clone --depth=1 "file://$(pwd)/server" client && > + > + # Add extra commits to the client so that the whole fetch takes more > + # than 1 request (due to negotiation) > + for i in $(seq 1 32) > + do > + test_commit -C client c$i > + done && > + > + git -C server checkout -b newbranch base && > + test_commit -C server client_wants && > + > + GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \ > + fetch origin newbranch && > + # Ensure that protocol v2 is used > + grep "git< version 2" trace > +' > + > # Test protocol v2 with 'http://' transport > # > . "$TEST_DIRECTORY"/lib-httpd.sh > diff --git a/upload-pack.c b/upload-pack.c > index 62a1000f44..de7de1de38 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -37,6 +37,9 @@ > #define CLIENT_SHALLOW (1u << 18) > #define HIDDEN_REF (1u << 19) > > +#define ALL_FLAGS (THEY_HAVE | OUR_REF | WANTED | COMMON_KNOWN | SHALLOW | \ > + NOT_SHALLOW | CLIENT_SHALLOW | HIDDEN_REF) > + > static timestamp_t oldest_have; > > static int deepen_relative; > @@ -1393,6 +1396,8 @@ int upload_pack_v2(struct repository *r, struct > argv_array *keys, > enum fetch_state state = FETCH_PROCESS_ARGS; > struct upload_pack_data data; > > + clear_object_flags(ALL_FLAGS); > + > git_config(upload_pack_config, NULL); > > upload_pack_data_init(); > -- > 2.19.0.271.gfe8321ec05.dirty >
Re: [PATCH v4] branch: introduce --show-current display option
On Wed, Oct 17, 2018 at 6:18 AM Johannes Schindelin
wrote:
> I realized yesterday that the &&-chain linting we use for every single
> test case takes a noticeable chunk of time:
>
> $ time ./t0006-date.sh --quiet
> real0m20.973s
> $ time ./t0006-date.sh --quiet --no-chain-lint
> real0m13.607s
>
> My suspicion: it is essentially the `(exit 117)` that adds about 100ms to
> every of those 67 test cases.
The subshell chain-linter adds a 'sed' and 'grep' invocation to each test which
doesn't help. (v1 of the subshell chain-linter only added a 'sed', but that
changed with v2.)
> With that in mind, I would like to suggest that we should start to be very
> careful about using subshells in our test suite.
You could disable the subshell chain-linter like this if you want test the
(exit 117) goop in isolation:
--- 8< ---
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3f95bfda60..48323e503c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -675,8 +675,7 @@ test_run_ () {
trace=
# 117 is magic because it is unlikely to match the exit
# code of other programs
- if $(printf '%s\n' "$1" | sed -f
"$GIT_BUILD_DIR/t/chainlint.sed" | grep -q '?![A-Z][A-Z]*?!') ||
- test "OK-117" != "$(test_eval_ "(exit 117) &&
$1${LF}${LF}echo OK-\$?" 3>&1)"
+ if test "OK-117" != "$(test_eval_ "(exit 117) &&
$1${LF}${LF}echo OK-\$?" 3>&1)"
then
error "bug in the test script: broken &&-chain or
run-away HERE-DOC: $1"
fi
--- 8< ---
Re: [PATCH v4] branch: introduce --show-current display option
Hi Eric,
On Tue, 16 Oct 2018, Eric Sunshine wrote:
> On Tue, Oct 16, 2018 at 7:09 PM Junio C Hamano wrote:
> > Eric Sunshine writes:
> > > This cleanup "checkout" needs to be encapsulated within a
> > > test_when_finished(), doesn't it? Preferably just after the "git
> > > checkout -b" invocation.
> >
> > In the meantime, here is what I'll have in 'pu' on top.
> >
> > diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> > @@ -119,12 +119,14 @@ test_expect_success 'git branch `--show-current`
> > works properly when tag exists'
> > cat >expect <<-\EOF &&
> > branch-and-tag-name
> > EOF
> > - test_when_finished "git branch -D branch-and-tag-name" &&
> > + test_when_finished "
> > + git checkout branch-one
> > + git branch -D branch-and-tag-name
> > + " &&
> > git checkout -b branch-and-tag-name &&
> > test_when_finished "git tag -d branch-and-tag-name" &&
> > git tag branch-and-tag-name &&
> > git branch --show-current >actual &&
> > - git checkout branch-one &&
> > test_cmp expect actual
> > '
>
> This make sense to me.
>
> > @@ -137,8 +139,7 @@ test_expect_success 'git branch `--show-current` works
> > properly with worktrees'
> > git worktree add worktree branch-two &&
> > (
> > git branch --show-current &&
> > - cd worktree &&
> > - git branch --show-current
> > + git -C worktree branch --show-current
> > ) >actual &&
> > test_cmp expect actual
> > '
>
> The subshell '(...)' could become '{...}' now that the 'cd' is gone,
> but that's a minor point.
Maybe not so minor.
I realized yesterday that the &&-chain linting we use for every single
test case takes a noticeable chunk of time:
$ time ./t0006-date.sh --quiet
# passed all 67 test(s)
1..67
real0m20.973s
user0m2.662s
sys 0m14.789s
$ time ./t0006-date.sh --quiet --no-chain-lint
# passed all 67 test(s)
1..67
real0m13.607s
user0m1.330s
sys 0m8.070s
My suspicion: it is essentially the `(exit 117)` that adds about 100ms to
every of those 67 test cases.
(Remember: a subshell requires a fork, and the `fork()` emulation on
Windows requires all kinds of things to be copied to a new process,
including memory and open file descriptors, before the `exec()` will undo
at least part of that.)
With that in mind, I would like to suggest that we should start to be very
careful about using subshells in our test suite.
Ciao,
Dscho
Re: [PATCH 3/3] pack-objects (mingw): initialize `packing_data` mutex in the correct spot
On Tue, Oct 16, 2018 at 02:02:52PM -0700, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin > > In 9ac3f0e5b3e4 (pack-objects: fix performance issues on packing large > deltas, 2018-07-22), a mutex was introduced that is used to guard the > call to set the delta size. This commit even added code to initialize > it, but at an incorrect spot: in `init_threaded_search()`, while the > call to `oe_set_delta_size()` (and hence to `packing_data_lock()`) can > happen in the call chain `check_object()` <- `get_object_details()` <- > `prepare_pack()` <- `cmd_pack_objects()`, which is long before the > `prepare_pack()` function calls `ll_find_deltas()` (which initializes > the threaded search). > > Another tell-tale that the mutex was initialized in an incorrect spot is > that the function to initialize it lives in builtin/, while the code > that uses the mutex is defined in a libgit.a header file. > > Let's use a more appropriate function: `prepare_packing_data()`, which > not only lives in libgit.a, but *has* to be called before the > `packing_data` struct is used that contains that mutex. Nicely explained. I think this is a good solution. Both before and after your patch, we do still take the lock even in single-threaded scenarios (the case you found where we are not yet in the delta search phase, but also when --threads=1). I think that should be fine. It looks like we do that with the other locks in pack-objects.c already. In index-pack.c, we check a threads_active flag before looking at the lock, which could be another possible solution. I doubt it's any faster, though (which is why I assume index-pack.c does it). Locking/unlocking a mutex should not really be much slower than checking the conditional flag in the first place. Which is all a roundabout way of saying "looks good to me". -Peff
Re: [PATCH v4] branch: introduce --show-current display option
On Fri, Oct 12, 2018 at 03:33:21PM +0200, Daniels Umanovskis wrote:
> Intended both for scripting and interactive/informative use.
> Unlike git branch --list, no filtering is needed to just get the
> branch name.
Are we going forward with advertising this as a scriptable alternative?
> + } else if (show_current) {
> + print_current_branch_name();
> + return 0;
Do we need the slightly different check done in
print_current_branch_name() ? A very similar check is already done early
in cmd_branch.
builtin/branch.c:671
head = resolve_refdup("HEAD", 0, _oid, NULL);
if (!head)
die(_("Failed to resolve HEAD as a valid ref."));
if (!strcmp(head, "HEAD"))
filter.detached = 1;
else if (!skip_prefix(head, "refs/heads/", ))
die(_("HEAD not found below refs/heads!"));
What's being proposed can be achieved with
+ } else if (show_current) {
+ if (!filter.detached)
+ puts(head);
+ return 0;
without failing tests.
--
Cheers,
Rafael Ascensão
[PATCH] test-tool: show tool list on error
Before we switched to one big test-tool binary, if you
forgot the name of a tool, you could use tab-completion in
the shell to get a hint. But these days, all you get is:
$ t/helper/test-tool approxidate
fatal: There is no test named 'approxidate'
and you're stuck reading the source code to find it. Let's
print a list of the available tools in this case.
Signed-off-by: Jeff King
---
Not really user-facing, but this bugged me enough earlier to write the
patch. ;)
Some of the individual tools have nice help, too (try
"t/helper/test-tool date", which shows the approxidate command I was
looking for), but some of them could probably stand to improve their
friendliness (try "t/helper/test-tool config"). I think it's fine for
people to improve them over time if and when they get annoyed.
t/helper/test-tool.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 6b5836dc1b..5df8b682aa 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -55,13 +55,23 @@ static struct test_cmd cmds[] = {
{ "write-cache", cmd__write_cache },
};
+static NORETURN void die_usage(void)
+{
+ size_t i;
+
+ fprintf(stderr, "usage: test-tool [args]\n");
+ for (i = 0; i < ARRAY_SIZE(cmds); i++)
+ fprintf(stderr, " %s\n", cmds[i].name);
+ exit(128);
+}
+
int cmd_main(int argc, const char **argv)
{
int i;
BUG_exit_code = 99;
if (argc < 2)
- die("I need a test name!");
+ die_usage();
for (i = 0; i < ARRAY_SIZE(cmds); i++) {
if (!strcmp(cmds[i].name, argv[1])) {
@@ -70,5 +80,6 @@ int cmd_main(int argc, const char **argv)
return cmds[i].fn(argc, argv);
}
}
- die("There is no test named '%s'", argv[1]);
+ error("there is no tool named '%s'", argv[1]);
+ die_usage();
}
--
2.19.1.790.g519f91cad4
Re: [RFC] revision: Add --sticky-default option
On Tue, Oct 16, 2018 at 11:24:38PM +0200, Andreas Gruenbacher wrote:
> here's a long-overdue update of my proposal from August 29:
>
> [RFC] revision: Don't let ^ cancel out the default
>
> Does this look more acceptable that my first shot?
I think it's going in the right direction.
The name "--sticky-default" did not immediately make clear to me what it
does. Is there some name that would be more obvious?
> Some commands like 'log' default to HEAD if no other revisions are
> specified on the command line or otherwise. Currently, excludes
> (^) cancel out that default, so when a command only excludes
> revisions (e.g., 'git log ^origin/master'), it will produce no output.
>
> With the --sticky-default option, the default becomes more "sticky" and
> is no longer canceled out by excludes.
>
> This is useful in wrappers that exclude certain revisions: for example,
> a simple alias l='git log --sticky-default ^origin/master' will show the
> revisions between origin/master and HEAD when invoked without arguments,
> and 'l foo' will show the revisions between origin/master and foo.
Your explanation makes sense.
> ---
> revision.c | 31 +++
> revision.h | 1 +
> t/t4202-log.sh | 6 ++
We'd probably want an update to Documentation/rev-list-options.txt (I
notice that "--default" is not covered there; that might be worth
fixing, too)>
> +static int has_revisions(struct rev_info *revs)
> +{
> + struct rev_cmdline_info *info = >cmdline;
> +
> + return info->nr != 0;
> +}
So this function is going to replace this flag:
> @@ -2401,8 +2423,6 @@ int setup_revisions(int argc, const char **argv, struct
> rev_info *revs, struct s
> argv_array_pushv(_data, argv + i);
> break;
> }
> - else
> - got_rev_arg = 1;
> }
Are we sure that every that hits that "else" is going to trigger
info->nr (and vice versa)?
If I say "--tags", I think we may end up with entries in revs->cmdline,
but would not have set got_rev_arg. That's captured separately in
revs->rev_input_given. But your cancel_default logic:
> @@ -2431,7 +2451,10 @@ int setup_revisions(int argc, const char **argv,
> struct rev_info *revs, struct s
> opt->tweak(revs, opt);
> if (revs->show_merge)
> prepare_show_merge(revs);
> - if (revs->def && !revs->pending.nr && !revs->rev_input_given &&
> !got_rev_arg) {
> + cancel_default = revs->sticky_default ?
> + has_interesting_revisions(revs) :
> + has_revisions(revs);
> + if (revs->def && !revs->rev_input_given && !cancel_default) {
doesn't handle that. So if I do:
git rev-list --count --sticky-default --default HEAD --not --tags
I should see everything in HEAD that's not tagged. But we don't even
look at cancel_default, because !revs->rev_input_given is not true.
I think you could solve that by making the logic more like:
if (revs->sticky_default)
cancel_default = has_interesting_revisions();
else
cancel_default = !revs->rev_input_given && !got_rev_arg;
which leaves the non-sticky case exactly as it is today.
> diff --git a/revision.h b/revision.h
> index 2b30ac270..570fa1a6d 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -92,6 +92,7 @@ struct rev_info {
>
> unsigned int early_output;
>
> + unsigned intsticky_default:1;
> unsigned intignore_missing:1,
> ignore_missing_links:1;
Maybe it would make sense to put this next to "const char *def"?
The bitfield would not be as efficient, but I don't think we care about
packing rev_info tightly.
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 153a50615..9517a65da 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -213,6 +213,12 @@ test_expect_success 'git show leaves list of
> commits as given' '
> test_cmp expect actual
> '
>
> +printf "sixth\nfifth\n" > expect
> +test_expect_success '--sticky-default ^' '
> + git log --pretty="tformat:%s" --sticky-default ^HEAD~2 > actual &&
> + test_cmp expect actual
> +'
Yuck, t4202 is a mix of older and newer styles. I'm OK with this as-is
because you've matched the surrounding code, but these days I'd probably
write:
test_expect_success '--sticky-default ^' '
{
echo sixth
echo fifth
} >expect &&
git log --format=%s --sticky-default ^HEAD~2 >actual &&
test_cmp expect actual
'
-Peff
Re: Recommendations for updating error() to error_errno()
On Wed, Oct 17, 2018 at 11:58:15AM +0800, 牛旭 wrote:
> Our team works on enhance logging practices by learning from historical log
> revisions in evolution.
> And we find three patches that update error(..., strerror(errmp)) to
> error_errno(...).
>
> While applying this rule to git-2.14.2, we find 9 missed spot in file
> refs/files-backend.c, 1 in apply.c, 2 in trace.c, 4 in dir-iterator.c:.
>
> Here are the missed spots:
> 1) Line 1734 in file git-2.14.2/refs/files-backend.c:
> ret = raceproof_create_file(path.buf, rename_tmp_log_callback, );
> if (ret) {
> if (errno == EISDIR)
> error("directory not empty: %s", path.buf);
> else
> error("unable to move logfile %s to %s: %s",
> tmp.buf, path.buf,
> strerror(cb.true_errno));
This cannot be converted naively. Using error_errno() will always show
the value of "errno", but here we are using a saved value in
cb.true_errno.
It might work to do:
errno = cb.true_errno;
error_errno(...);
but you would first have to check if the function is depending on the
value of errno for other reasons (not to mention that it kind of defeats
the purpose of error_errno() being a shorthand).
> 2) Line 1795 in file git-2.14.2/refs/files-backend.c:
> if (log && rename(sb_oldref.buf, tmp_renamed_log.buf)) {
> ret = error("unable to move logfile logs/%s to logs/"TMP_RENAMED_LOG":
> %s",
> oldrefname, strerror(errno));
> goto out;
> }
But this one, for example, would be fine.
-Peff
Re: [PATCH v2] builtin/branch.c: remove useless branch_get
On Tue, Oct 16, 2018 at 10:54:28PM +0800, Tao Qingyun wrote: > branch_get sometimes returns current_branch, which can be NULL (e.g., if > you're on a detached HEAD). Try: > > $ git branch HEAD > fatal: no such branch 'HEAD' > > $ git branch '' > fatal: no such branch '' > > However, it seems weird that we'd check those cases here (and provide > such lousy messages). And indeed, dropping that and letting us > eventually hit create_branch() gives a much better message: > > $ git branch HEAD > fatal: 'HEAD' is not a valid branch name. > > $ git branch '' > fatal: '' is not a valid branch name. This explanation is perfect, of course. ;) I still wondered if you had another motivation hinted at in your original mail, though (some weirdness with running branch_get early). It's OK if there isn't one, but I just want to make sure we capture all of the details. Other than that question, the patch looks good to me. -Peff
Re: [PATCH 15/16] commit-reach: make can_all_from_reach... linear
On Sun, Oct 14, 2018 at 04:29:06PM +0200, René Scharfe wrote: > Anyway, drove the generative approach a bit further, and came up with > the new DEFINE_SORT below. I'm unsure about the name; perhaps it should > be called DEFINE_SORT_BY_COMPARE_FUNCTION_BODY, but that's a bit long. > It handles casts and const attributes behind the scenes and avoids > repetition, but looks a bit weird, as it is placed where a function > signature would go. > > Apart from that the macro is simple and doesn't use any tricks or > added checks. It just sets up boilerplate functions to offer type-safe > sorting. > > diffcore-rename.c and refs/packed-backend.c receive special treatment in > the patch because their compare functions are used outside of sorting as > well. I made them take typed pointers nevertheless and used them from > DEFINE_SORT; the wrapper generated by that macro is supposed to be > private. Given that such reuse is rare and I think we don't need a way > to make it public. > > What do y'all think about this direction? I think it's the best we're likely to do, and is an improvement on the status quo. The patch looks overall sane to me. I think DEFINE_SORT() is a fine name. I think given a macro parameter "foo" you could generate sort_by_foo() and compare_foo(), which would eliminate the extra layer in those two cases you mentioned. But I'm also fine with the approach you've shown here. -Peff
Recommendations for adding strerror() in log statament
Hi,
Our team works on enhance logging practices by learning from historical log
revisions in evolution.
And we find 2 patches that add strerror() to the log statement which is printed
when the return value of commit_lock_file() is smaller than 0.
While applying this rule to git-2.14.2, we find 3 missed spots.
We suggest to add strerror() or just use error_errno() instead of error().
Here are the missed spots:
1) Line 307 in file git-2.14.2/sequencer.c:
static int write_message(const void *buf, size_t len, const char *filename,
int append_eol)
{
...
if (append_eol && write(msg_fd, "\n", 1) < 0) {
rollback_lock_file(_file);
return error_errno(_("could not write eol to '%s'"), filename);
}
if (commit_lock_file(_file) < 0) {
rollback_lock_file(_file);
return error(_("failed to finalize '%s'."), filename);
}
2) Line 1582 in file git-2.14.2/sequencer.c:
static int save_head(const char *head)
{
...
strbuf_addf(, "%s\n", head);
if (write_in_full(fd, buf.buf, buf.len) < 0) {
rollback_lock_file(_lock);
return error_errno(_("could not write to '%s'"),
git_path_head_file());
}
if (commit_lock_file(_lock) < 0) {
rollback_lock_file(_lock);
return error(_("failed to finalize '%s'."),
git_path_head_file());
}
3) Line 1706 in file git-2.14.2/sequencer.c:
static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
{
...
if (write_in_full(fd, todo_list->buf.buf + offset,
todo_list->buf.len - offset) < 0)
return error_errno(_("could not write to '%s'"), todo_path);
if (commit_lock_file(_lock) < 0)
return error(_("failed to finalize '%s'."), todo_path);
Following are the 2 patches that support our opinion:
1) Line 2147 in file git-2.6.4/config.c:
if (commit_lock_file(lock) < 0) {
- error("could not commit config file %s", config_filename);
+ error("could not write config file %s: %s", config_filename,
+ strerror(errno));
ret = CONFIG_NO_WRITE;
lock = NULL;
goto out_free;
}
2) Line 2333 in file git-2.6.4/config.c:
unlock_and_out:
if (commit_lock_file(lock) < 0)
- ret = error("could not commit config file %s", config_filename);
+ ret = error("could not write config file %s: %s",
+ config_filename, strerror(errno));
out:
free(filename_buf);
Thanks for reading, we are looking forward to your reply about the correctness
of our suggestion~
May you a good day ^^
Best regards,
Xu
Re: [PATCH] Typo `dashes ?` -> `dashes?`
On Mon, Oct 15, 2018 at 09:31:54PM +0200, Jacques Bodin-Hullin wrote: > I've just updated the PR with the `error(` change. > > Also I did the change for the question mark at the end, because you are > right, when we read it, it sounds better. > > Do I need to put back the patch in this thread? It's here: > https://patch-diff.githubusercontent.com/raw/git/git/pull/540.patch The commit looks good, though you may want to expand the commit message a little (it's not really a typo, but more of a style fix). The next step would normally be to send the patch to the list (ideally in this thread), with "[PATCH v2]" in the subject. It looks like you're using SubmitGit. I don't remember offhand how it works for sending re-rolls like this, but I think it's possible. If you run into trouble, you might try instead using GitGitGadget, which is a similar PR->mail gateway that is a little more featureful: https://github.com/gitgitgadget/gitgitgadget -Peff
