Re: [PATCH/RFC/BUG] unpack-trees.c: do not use "the_index"
On Fri, Jun 1, 2018 at 10:03 PM, Duy Nguyen wrote: > On Fri, Jun 1, 2018 at 8:34 PM, Elijah Newren wrote: >> On Fri, Jun 1, 2018 at 9:11 AM, Nguyễn Thái Ngọc Duy >> wrote: >>> This is more of a bug report than an actual fix because I'm not sure >>> if "o->src_index" is always the correct answer instead of "the_index" >>> here. But this is very similar to 7db118303a (unpack_trees: fix >>> breakage when o->src_index != o->dst_index - 2018-04-23) and could >>> potentially break things again... I'm pretty sure your patch is correct. Adding Brandon Williams to the cc for comment since his patches came up in the analysis below... >> Actually, I don't think the patch will break anything in the current >> code. Currently, all callers of unpack_trees() (even merge recursive >> which uses different src_index and dst_index now) set src_index = >> _index. So, these changes should continue to work as before (with >> a minor possible exception of merge-recursive calling into other >> functions from unpack-trees.c after unpack_trees() has finished..). >> That's not to say that your patch is bug free, just that I think any >> bugs shouldn't be triggerable from the current codebase. > > Ah.. I thought merge-recursive would do fancier things and used some > temporary index. Good to know. Well, it does does use a temporary index, but for dst_index rather than src_index. It then does some fancier things, but not until the call to unpack_trees() is over. In particular, at that point, it swaps the_index and tmp_index, reversing their roles so that now tmp_index is the original index and the_index becomes the result after unpack_trees() is run. That's done because I later want to use the original index for calling verify_uptodate(). verify_uptodate() is then used for was_tracked_and_matches() and was_tracked(). Anyway, the whole upshot of this is: * merge-recursive uses src_index == _index for the unpack_trees() call. * merge-recursive uses src_index == o->orig_index for subsequent calls to verify_uptodate(), but verify_uptodate() doesn't call into any of the sites you have modified. Further: * Every other existing caller of unpack-trees in the code sets src_index == _index, so this won't break any of them. * There are only two callers in the codebase that set dst_index to something other than _index -- diff-lib.c (which sets it to NULL) and merge-recursive (which does the stuff described above). So, having done that analysis, I am now pretty convinced your patch won't break anything. That's one half... >> Also, if any of the changes you made are wrong, what was there before >> was also clearly wrong. So I think we're at least no worse off. >> >> But, I agree, it's not easy to verify what the correct thing should be >> in all cases. I'll try to take a closer look in the next couple days. > > Thanks. I will also stare at this code some more in the next couple > days trying to remember what these functions do. Your patch has two divisible parts: 1) Your modifications to * clear_ce_flags_1() * clear_ce_flags_dir() * clear_ce_flags() * mark_new_skip_worktree() The clear_ce_flags*() functions are only called by each other and by mark_new_skip_worktree(), which in turn is only called from unpack_trees(). Also, in all of these, your change ends up only modifying what index_state is passed to is_excluded_from_list(). 2) Your modifications to * verify_clean_subdirectory() * check_ok_to_remove() In this case, the former is only called by the latter, and the latter ends up only being called (via verify_absent_1()) from verify_absent() and verify_absent_sparse(). I'll address each, in reverse order. 2) The stuff that affects verify_absent() While verify_absent() is not called from merge-recursive right now, it was something I wanted to use in the future for very similar reasons that verify_uptodate() started being called directly from merge-recursive. In particular, if the rewrite of merge-recursive[A] I want to do sets index_only when calling unpack_trees(), then does the whole merge without touching the worktree, then at the end goes to update the working tree, it will need to do extra checks. verify_absent() will come in handy as one of those extra checks. For that case, using the_index (the new index just created with lots of changes) would be wrong in all the same ways that using the_index caused massive problems for was_tracked() in merge-recursive (e.g. the blow up of when Junio merged the original directory rename detection series into master and subsequently reverted it); we'd instead want src_index (which was the index that existed when merge was called) instead. So, with this patch you've fixed some important bugs that I would have hit later. [A] sidenote: see https://public-inbox.org/git/xmqqk1ydkbx0@gitster.mtv.corp.google.com/ for more details 1) mark_new_skip_worktree() ... is_excluded_from_list(). Sadly, I'm not very familiar with the skip_worktree and sparse
git glob pattern in .gitignore and git command
To ignore all .js file under a directory `lib', I can use "lib/**/js" to match them. But when using git command such as "git add", using "git add lib/\*.js" is sufficient. Why is this difference in glob mode? I have heard that there are many different glob mode out there (e.g., bash has many different glob mode). So, which classes of glob mode does these two belong to? Do they have a name? Yubin
Re: [PATCH v3 11/20] commit-graph: verify root tree OIDs
Derrick Stolee writes:
> On 5/30/2018 6:24 PM, Jakub Narebski wrote:
[...]
>> NOTE: we will be checking Commit Data chunk; I think it would be good
>> idea to verify that size of Commit Data chunk matches (N * (H + 16) bytes)
>> that format gives us, so that we don't accidentally red outside of
>> memory if something got screwed up (like number of commits being wrong,
>> or file truncated).
>
> This is actually how we calculate 'num_commits' during
> load_commit_graph_one():
>
> if (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP)
> {
> graph->num_commits = (chunk_offset - last_chunk_offset)
> / graph->hash_len;
> }
>
> So, if the chunk doesn't match N*(H+16), we detect this because
> FANOUT[255] != N.
>
> (There is one caveat here: (chunk_offset - last_chunk_offset) may not
> be a multiple of hash_len, and "accidentally" truncate to N in the
> division. I'll add more careful checks for this.)
I have thought for some reason that number of commits N was stored
somewhere directly in the commit-graph header.
Anyway we have three places that we can calculate (or simply read in
case of OID Fanour chunk) the number of commits:
- FANOUT[255] == N
- OID Lookup size = (N * H bytes)
- N = (OID Lookup size) / hash_len
- (OID Lookup size) % hash_len == 0
- Commit Data size = (N * (H + 16) bytes)
- N = (Commir Data size) / (hash_len + 16)
- (Commir Data size) % (hash_len + 16) == 0
>
> We also check out-of-bounds offsets in that method.
Good.
Best,
--
Jakub Narębski
Re: [PATCH v3 07/20] commit-graph: verify catches corrupt signature
Derrick Stolee writes: > On 5/28/2018 10:05 AM, Jakub Narebski wrote: >> Derrick Stolee writes: [...] >>> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh >>> index 6ca451dfd2..bd64481c7a 100755 >>> --- a/t/t5318-commit-graph.sh >>> +++ b/t/t5318-commit-graph.sh >>> @@ -235,9 +235,52 @@ test_expect_success 'perform fast-forward merge in >>> full repo' ' >>> test_cmp expect output >>> ' >>> +# the verify tests below expect the commit-graph to contain >>> +# exactly the commits reachable from the commits/8 branch. >>> +# If the file changes the set of commits in the list, then the >>> +# offsets into the binary file will result in different edits >>> +# and the tests will likely break. >>> + >>> test_expect_success 'git commit-graph verify' ' >>> cd "$TRASH_DIRECTORY/full" && >>> + git rev-parse commits/8 | git commit-graph write --stdin-commits && >>> git commit-graph verify >output >>> ' >> I don't quite understand what the change is meant to do. > > This gives us a constant commit-graph file to work with in the later tests. > > To get the "independent test" structure you want for the tests that > are coming, we need to do one of the following: > > 1. Write a new commit-graph file for every test (slows things down). Or check if correct graph-file exists, and if it doesn't only then write a new commit-graph file (like I have proposed elsewhere in this thread). Barring this, I think it would be better if the preparation step was separated into a 'setup ' step, so that one can easier select which tests to run, at least by hand. > 2. Do all corruption/verify checks in a single test (reduces the > information from a failed test, as it only reports the first failure). > > I don't like either of these options, so I went with this "prepare" step. These are not the only possible options. >> Also, as I said earlier, I'd prefer if tests were as indpendent of each >> other as possible, to make running individual tests (e.g. only >> previously falling tests) easier. >> >> I especially do not like mixing running actual test with setting up the >> repository for future tests, as here.
[PATCH v7 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875
It's been time. Thank you for parience. Changes: * did most of the changes proposed * rebase to newer master (latest conflicting change is addition of combined test helper) * make tests which cover, hopefully, all cases. * handle incorectly truncated input also in receive-pack. Considering the complications pointed out by Jeff, it just filters the input in the frontend process. I hope it is acceptable thing to do. Max Kirillov (2): http-backend: respect CONTENT_LENGTH as specified by rfc3875 http-backend: respect CONTENT_LENGTH for receive-pack Makefile| 1 + config.c| 2 +- config.h| 1 + http-backend.c | 86 +++-- t/helper/test-print-larger-than-ssize.c | 11 ++ t/helper/test-tool.c| 1 + t/helper/test-tool.h| 1 + t/t5560-http-backend-noserver.sh| 13 ++ t/t5562-http-backend-content-length.sh | 155 t/t5562/invoke-with-content-length.pl | 30 + 10 files changed, 291 insertions(+), 10 deletions(-) create mode 100644 t/helper/test-print-larger-than-ssize.c create mode 100755 t/t5562-http-backend-content-length.sh create mode 100755 t/t5562/invoke-with-content-length.pl -- 2.17.0.1185.g782057d875
[PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack
Push passes to another commands, as described in
https://public-inbox.org/git/20171129032214.gb32...@sigill.intra.peff.net/
As it gets complicated to correctly track the data length, instead transfer
the data through parent process and cut the pipe as the specified length is
reached. Do it only when CONTENT_LENGTH is set, otherwise pass the input
directly to the forked commands.
Add tests for cases:
* CONTENT_LENGTH is set, script's stdin has more data, with all combinations
of variations: fetch or push, plain or compressed body, correct or truncated
input.
* CONTENT_LENGTH is specified to a value which does not fit into ssize_t.
Helped-by: Junio C Hamano
Signed-off-by: Max Kirillov
---
Makefile| 1 +
http-backend.c | 49 ++--
t/helper/test-print-larger-than-ssize.c | 11 ++
t/helper/test-tool.c| 1 +
t/helper/test-tool.h| 1 +
t/t5560-http-backend-noserver.sh| 13 ++
t/t5562-http-backend-content-length.sh | 155
t/t5562/invoke-with-content-length.pl | 30 +
8 files changed, 250 insertions(+), 11 deletions(-)
create mode 100644 t/helper/test-print-larger-than-ssize.c
create mode 100755 t/t5562-http-backend-content-length.sh
create mode 100755 t/t5562/invoke-with-content-length.pl
diff --git a/Makefile b/Makefile
index f181687250..93dc4bc23b 100644
--- a/Makefile
+++ b/Makefile
@@ -678,6 +678,7 @@ TEST_BUILTINS_OBJS += test-mergesort.o
TEST_BUILTINS_OBJS += test-mktemp.o
TEST_BUILTINS_OBJS += test-online-cpus.o
TEST_BUILTINS_OBJS += test-path-utils.o
+TEST_BUILTINS_OBJS += test-print-larger-than-ssize.o
TEST_BUILTINS_OBJS += test-prio-queue.o
TEST_BUILTINS_OBJS += test-read-cache.o
TEST_BUILTINS_OBJS += test-ref-store.o
diff --git a/http-backend.c b/http-backend.c
index 3066697a24..78a588c551 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -351,23 +351,22 @@ static ssize_t get_content_length(void)
return val;
}
-static ssize_t read_request(int fd, unsigned char **out)
+static ssize_t read_request(int fd, unsigned char **out, ssize_t req_len)
{
- ssize_t req_len = get_content_length();
-
if (req_len < 0)
return read_request_eof(fd, out);
else
return read_request_fixed_len(fd, req_len, out);
}
-static void inflate_request(const char *prog_name, int out, int buffer_input)
+static void inflate_request(const char *prog_name, int out, int buffer_input,
ssize_t req_len)
{
git_zstream stream;
unsigned char *full_request = NULL;
unsigned char in_buf[8192];
unsigned char out_buf[8192];
unsigned long cnt = 0;
+ ssize_t req_remaining_len = req_len;
memset(, 0, sizeof(stream));
git_inflate_init_gzip_only();
@@ -379,11 +378,18 @@ static void inflate_request(const char *prog_name, int
out, int buffer_input)
if (full_request)
n = 0; /* nothing left to read */
else
- n = read_request(0, _request);
+ n = read_request(0, _request, req_len);
stream.next_in = full_request;
} else {
- n = xread(0, in_buf, sizeof(in_buf));
+ ssize_t buffer_len;
+ if (req_remaining_len < 0 || req_remaining_len >
sizeof(in_buf))
+ buffer_len = sizeof(in_buf);
+ else
+ buffer_len = req_remaining_len;
+ n = xread(0, in_buf, buffer_len);
stream.next_in = in_buf;
+ if (req_remaining_len >= 0)
+ req_remaining_len -= n;
}
if (n <= 0)
@@ -416,10 +422,10 @@ static void inflate_request(const char *prog_name, int
out, int buffer_input)
free(full_request);
}
-static void copy_request(const char *prog_name, int out)
+static void copy_request(const char *prog_name, int out, ssize_t req_len)
{
unsigned char *buf;
- ssize_t n = read_request(0, );
+ ssize_t n = read_request(0, , req_len);
if (n < 0)
die_errno("error reading request body");
if (write_in_full(out, buf, n) < 0)
@@ -428,6 +434,24 @@ static void copy_request(const char *prog_name, int out)
free(buf);
}
+static void pipe_fixed_length(const char *prog_name, int out, size_t req_len)
+{
+ unsigned char buf[8192];
+ size_t remaining_len = req_len;
+
+ while (remaining_len > 0) {
+ size_t chunk_length = remaining_len > sizeof(buf) ? sizeof(buf)
: remaining_len;
+ size_t n = xread(0, buf, chunk_length);
+ if (n < 0)
+ die_errno("Reading request failed");
+ if (write_in_full(out, buf, n)
[PATCH v7 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875
http-backend reads whole input until EOF. However, the RFC 3875 specifies
that a script must read only as many bytes as specified by CONTENT_LENGTH
environment variable. Web server may exercise the specification by not closing
the script's standard input after writing content. In that case http-backend
would hang waiting for the input. The issue is known to happen with
IIS/Windows, for example.
Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
the whole input until EOF. If the variable is not defined, keep older behavior
of reading until EOF because it is used to support chunked transfer-encoding.
Signed-off-by: Florian Manschwetus
[mk: fixed trivial build failures and polished style issues]
Helped-by: Junio C Hamano
Signed-off-by: Max Kirillov
---
config.c | 2 +-
config.h | 1 +
http-backend.c | 43 ++-
3 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index c698988f5e..4148a3529d 100644
--- a/config.c
+++ b/config.c
@@ -853,7 +853,7 @@ int git_parse_ulong(const char *value, unsigned long *ret)
return 1;
}
-static int git_parse_ssize_t(const char *value, ssize_t *ret)
+int git_parse_ssize_t(const char *value, ssize_t *ret)
{
intmax_t tmp;
if (!git_parse_signed(value, ,
maximum_signed_value_of_type(ssize_t)))
diff --git a/config.h b/config.h
index ef70a9cac1..c143a1b634 100644
--- a/config.h
+++ b/config.h
@@ -48,6 +48,7 @@ extern void git_config(config_fn_t fn, void *);
extern int config_with_options(config_fn_t fn, void *,
struct git_config_source *config_source,
const struct config_options *opts);
+extern int git_parse_ssize_t(const char *, ssize_t *);
extern int git_parse_ulong(const char *, unsigned long *);
extern int git_parse_maybe_bool(const char *);
extern int git_config_int(const char *, const char *);
diff --git a/http-backend.c b/http-backend.c
index 88d2a9bc40..3066697a24 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -283,7 +283,7 @@ static struct rpc_service *select_service(struct strbuf
*hdr, const char *name)
* hit max_request_buffer we die (we'd rather reject a
* maliciously large request than chew up infinite memory).
*/
-static ssize_t read_request(int fd, unsigned char **out)
+static ssize_t read_request_eof(int fd, unsigned char **out)
{
size_t len = 0, alloc = 8192;
unsigned char *buf = xmalloc(alloc);
@@ -320,6 +320,47 @@ static ssize_t read_request(int fd, unsigned char **out)
}
}
+static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char
**out)
+{
+ unsigned char *buf = NULL;
+ ssize_t cnt = 0;
+
+ if (max_request_buffer < req_len) {
+ die("request was larger than our maximum size (%lu): "
+ "%" PRIuMAX "; try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+ max_request_buffer, (uintmax_t)req_len);
+ }
+
+ buf = xmalloc(req_len);
+ cnt = read_in_full(fd, buf, req_len);
+ if (cnt < 0) {
+ free(buf);
+ return -1;
+ }
+ *out = buf;
+ return cnt;
+}
+
+static ssize_t get_content_length(void)
+{
+ ssize_t val = -1;
+ const char *str = getenv("CONTENT_LENGTH");
+
+ if (str && !git_parse_ssize_t(str, ))
+ die("failed to parse CONTENT_LENGTH: %s", str);
+ return val;
+}
+
+static ssize_t read_request(int fd, unsigned char **out)
+{
+ ssize_t req_len = get_content_length();
+
+ if (req_len < 0)
+ return read_request_eof(fd, out);
+ else
+ return read_request_fixed_len(fd, req_len, out);
+}
+
static void inflate_request(const char *prog_name, int out, int buffer_input)
{
git_zstream stream;
--
2.17.0.1185.g782057d875
Re: [PATCH v3 06/20] commit-graph: add 'verify' subcommand
Derrick Stolee writes:
> On 5/27/2018 6:55 PM, Jakub Narebski wrote:
>> Derrick Stolee writes:
[...]
>>> +static int verify_commit_graph_error;
>>> +
>>> +static void graph_report(const char *fmt, ...)
>>> +{
>>> + va_list ap;
>>> + struct strbuf sb = STRBUF_INIT;
>>> + verify_commit_graph_error = 1;
>>> +
>>> + va_start(ap, fmt);
>>> + strbuf_vaddf(, fmt, ap);
>>> +
>>> + fprintf(stderr, "%s\n", sb.buf);
>>> + strbuf_release();
>>> + va_end(ap);
>>
>> Why do you use strbuf_vaddf + fprintf instead of straighforward
>> vfprintf (or function instead of variable-level macro)?
>>
>> Is it because of [string] safety?
>
> It's because I've never used this variable-parameter thing before and
> found a different example.
>
> I'll use vfprintf() in v4, as it is simpler.
All right, if it is not dangerous, then simpler is better.
Sidenote: such error messaging is often handled by variadic macros,
e.g.:
#define eprintf(...) fprintf(stderr, __VA_ARGS__)
[...]
>>> diff --git a/commit-graph.h b/commit-graph.h
>>> index 96cccb10f3..71a39c5a57 100644
>>> --- a/commit-graph.h
>>> +++ b/commit-graph.h
>>> @@ -53,4 +53,6 @@ void write_commit_graph(const char *obj_dir,
>>> int nr_commits,
>>> int append);
>>> +int verify_commit_graph(struct commit_graph *g);
>>> +
>> Why does this need to be exported? I think it is not used outside of
>> commit-graph.c, isn't it?
>
> Used by builtin/commit-graph.c
Ah, true.
[...]
>>> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
>>> index 77d85aefe7..6ca451dfd2 100755
>>> --- a/t/t5318-commit-graph.sh
>>> +++ b/t/t5318-commit-graph.sh
>>> @@ -11,6 +11,11 @@ test_expect_success 'setup full repo' '
>>> objdir=".git/objects"
>>> '
>>> +test_expect_success 'verify graph with no graph file' '
>>> + cd "$TRASH_DIRECTORY/full" &&
>>> + git commit-graph verify
>>> +'
>>> +
>>> test_expect_success 'write graph with no packs' '
>>> cd "$TRASH_DIRECTORY/full" &&
>>> git commit-graph write --object-dir . &&
>>> @@ -230,4 +235,9 @@ test_expect_success 'perform fast-forward merge in full
>>> repo' '
>>> test_cmp expect output
>>> '
>>> +test_expect_success 'git commit-graph verify' '
>>> + cd "$TRASH_DIRECTORY/full" &&
>>> + git commit-graph verify >output
>>> +'
>> Those are tests with nearly the same code, but they are (by their
>> descriptions) testing different things. This means that they rely on
>> side effects of earlier tests.
>>
>> This is suboptimal, as it means that it would be impossible or very
>> difficult to run individual tests (e.g. with GIT_SKIP_TESTS environment
>> variable, or with an individual test suite --run option), unless you
>> know which tests setup the repository state for later tests.
>>
>> It also means that running only failed tests with prove
>> --state=failed,save or equivalently with
>>
>>$ make DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS='--state=failed,save' test
>>
>> wouldn't work correctly.
>>
>> As Johannes Schindelin (alias Dscho) said in latest Git Rev News
>> interview: https://git.github.io/rev_news/2018/05/16/edition-39/
>>
>> JS> We have a test suite where debugging a regression may mean that you
>> JS> have to run 98 test cases before the failing one every single time in
>> JS> the edit/compile/debug cycle, because the 99th test case may depend on
>> JS> a side effect of at least one of the preceding test cases. Git’s test
>> JS> suite is so not [21st century best practices][1].
>> JS>
>> JS> [1]:
>> https://www.slideshare.net/BuckHodges/lessons-learned-doing-devops-at-scale-at-microsoft
>>
>>
>> I think can be solved quite efficiently by creating and using shell
>> function, or two shell functions, which would either:
>>
>> * rename commit-graph file to some other temporary name if it exists,
>> and move it back after the test.
>> * create commit-graph file if it does not exist.
>>
>> For example (untested):
>>
>>prepare_no_commit_graph() {
>> mv .git/info/commit-graph .git/info/commit-graph.away &&
>> test_when_finished "mv .git/info/commit-graph.away
>> .git/info/commit-graph"
>>}
>>
>>prepare_commit_graph() {
>> if ! test -f ".git/info/commit-graph"
>> then
>> git commit-graph write
>> fi
>>}
>>
>> Or something like that.
>
> Do we have a way to run individual steps of the test suite? I am
> unfamiliar with that process.
The t/README describes three such ways in "Skipping Tests" section:
- GIT_SKIP_TESTS environment variable, which can either can match the
"t[0-9]{4}" part to skip the whole test, or t[0-9]{4} followed by
".$number" to say which particular test to skip
- For an individual test suite --run could be used to specify that
only some tests should be run or that some tests should be
excluded from a run (the latter with '!' prefix).
- 'prove' harness can also run individual tests; one of more useful
options is --state, which for example would allow to
Re: does a stash *need* any reference to the branch on which it was created?
On 06/02, Robert P. J. Day wrote:
>
> i realize that, when you "git stash push", stash graciously saves
> the branch you were on as part of the commit message, but does any
> subsequent stash operation technically *need* that branch name?
$ git stash list
stash@{0}: WIP on master: 4e5a9c0166 checkout & worktree: introduce
checkout.defaultRemote
^^
Do you mean this branch name? If so, no, afaik nothing in git stash
needs that. It's merely a convenience for the user so they know which
branch the stash was based on.
> it doesn't seem like it -- even "git stash branch" really only needs
> the commit that was the basis of that stash to create the new branch.
Correct, and it knows that by checking the parents of the stash (note
that a stash is represented internally as just a commit):
$ git cat-file commit stash@{0}
∞
tree 9fc2608506404bebdeb0aea54e8c76944ae88a1a
parent 4e5a9c01669919bcc2452e8e2491ee31dbf647fc
parent 9e457faad129f832ce0070dcfd1f4cfd3f322df3
author Thomas Gummerer 1527971565 +0100
committer Thomas Gummerer 1527971565 +0100
WIP on master: 4e5a9c0166 checkout & worktree: introduce
checkout.defaultRemote
The first parent here is the commit the stash is based on, so that's
what 'git stash branch' is using to base the new branch on.
> so, does any stash operation actually need the originating branch
> name? (i'm guessing no, but i've been wrong before.)
>
> rday
>
> --
>
>
> Robert P. J. Day Ottawa, Ontario, CANADA
> http://crashcourse.ca/dokuwiki
>
> Twitter: http://twitter.com/rpjday
> LinkedIn: http://ca.linkedin.com/in/rpjday
>
Proposal
-- Hello I have been trying to contact you. Did you get my business proposal? Best Regards, Miss.Victoria Mehmet
Re: [PATCH v3 02/20] commit-graph: fix GRAPH_MIN_SIZE
"brian m. carlson" writes: > On Sat, May 26, 2018 at 08:46:09PM +0200, Jakub Narebski wrote: >> One issue: in the future when Git moves to NewHash, it could encounter >> then both commit-graph files using SHA-1 and using NewHash. What about >> GRPH_OID_LEN then: for one of those it would be incorrect. Unless it is >> about minimal length of checksum, that is we assume that NewHash would >> be longer than SHA-1, but ten why name it GRAPH_OID_LEN? > > My proposal is that whatever we're using in the .git directory is > consistent. If we're using SHA-1 for objects, then everything is SHA-1. > If we're using NewHash for objects, then all data is stored in NewHash > (except translation tables and such). Any conversions between SHA-1 and > NewHash require a lookup through the standard techniques. > > I agree that here it would be more helpful if it were a reference to > the_hash_algo, and I've applied a patch to my object-id-part14 series to > make that conversion. All right, I can agree that it would make most sense to always use SHA-1 for OID, or always use NewHash for objects. This would make commit-graph file with SHA-1 hash invalid for NewHash-using Git version. It would be nice, however, to avoid having to redo all the hard work, like calculating generation numbers (from old commit-graph file, or from server that does not support NewHash yet -- the latter is not implemented, but IIUC planned feature). But we can do it with explicit conversion step, e.g. 'git commit-graph convert' or 'upgrade'. But all that is in the future. -- Jakub Narębski
does a stash *need* any reference to the branch on which it was created?
i realize that, when you "git stash push", stash graciously saves the branch you were on as part of the commit message, but does any subsequent stash operation technically *need* that branch name? it doesn't seem like it -- even "git stash branch" really only needs the commit that was the basis of that stash to create the new branch. so, does any stash operation actually need the originating branch name? (i'm guessing no, but i've been wrong before.) rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: [RFC PATCH 4/6] commit-graph: avoid writing when repo is shallow
Derrick Stolee writes: > On 5/31/2018 10:30 PM, Junio C Hamano wrote: >> Derrick Stolee writes: >> >>> Shallow clones do not interact well with the commit-graph feature for >>> several reasons. Instead of doing the hard thing to fix those >>> interactions, instead prevent reading or writing a commit-graph file for >>> shallow repositories. >> >> The latter instead would want to vanish, I would guess. > > Do you mean that we should call destroy_commit_graph() if we detect a > shallow repository during write_commit_graph(), then I can make that > change. I think Junio meant here the "instead" word, because you have it twice in the second sentence of quoted paragraph. -- Jakub Narębski
Re: [PATCH v3 20/20] commit-graph: update design document
Derrick Stolee writes: > The commit-graph feature is now integrated with 'fsck' and 'gc', > so remove those items from the "Future Work" section of the > commit-graph design document. It is always nice to have such commit as a summary what was done in the series, and to have up to date roadmap. > > Also remove the section on lazy-loading trees, as that was completed > in an earlier patch series. Admittedly, this part could have been sent in a separate patch at the start of the series, but it doesn't matter at all; no need for extra work. > > Signed-off-by: Derrick Stolee > --- > Documentation/technical/commit-graph.txt | 22 -- > 1 file changed, 22 deletions(-) > > diff --git a/Documentation/technical/commit-graph.txt > b/Documentation/technical/commit-graph.txt > index e1a883eb46..c664acbd76 100644 > --- a/Documentation/technical/commit-graph.txt > +++ b/Documentation/technical/commit-graph.txt > @@ -118,9 +118,6 @@ Future Work > - The commit graph feature currently does not honor commit grafts. This can >be remedied by duplicating or refactoring the current graft logic. > > -- The 'commit-graph' subcommand does not have a "verify" mode that is > - necessary for integration with fsck. > - All right. > - After computing and storing generation numbers, we must make graph >walks aware of generation numbers to gain the performance benefits they >enable. This will mostly be accomplished by swapping a commit-date-ordered > @@ -130,25 +127,6 @@ Future Work > - 'log --topo-order' > - 'tag --merged' > > -- Currently, parse_commit_gently() requires filling in the root tree > - object for a commit. This passes through lookup_tree() and consequently > - lookup_object(). Also, it calls lookup_commit() when loading the parents. > - These method calls check the ODB for object existence, even if the > - consumer does not need the content. For example, we do not need the > - tree contents when computing merge bases. Now that commit parsing is > - removed from the computation time, these lookup operations are the > - slowest operations keeping graph walks from being fast. Consider > - loading these objects without verifying their existence in the ODB and > - only loading them fully when consumers need them. Consider a method > - such as "ensure_tree_loaded(commit)" that fully loads a tree before > - using commit->tree. All right, this is about the change done in previous series. > - > -- The current design uses the 'commit-graph' subcommand to generate the > graph. > - When this feature stabilizes enough to recommend to most users, we should > - add automatic graph writes to common operations that create many commits. > - For example, one could compute a graph on 'clone', 'fetch', or 'repack' > - commands. All right; actually it was done by augmenting 'gc' instead. > - > - A server could provide a commit graph file as part of the network protocol >to avoid extra calculations by clients. This feature is only of benefit if >the user is willing to trust the file, because verifying the file is > correct Good work, -- Jakub Narębski
Re: [PATCH v3 19/20] gc: automatically write commit-graph files
Derrick Stolee writes:
> The commit-graph file is a very helpful feature for speeding up git
> operations. In order to make it more useful, write the commit-graph file
> by default during standard garbage collection operations.
I think you meant here "make it possible to write the commit-graph file
during standard garbage collection operations." (i.e. add "make it
possible" because it hides behind new config option, and remove "by
default" because currently it is not turned on by default).
>
> Add a 'gc.commitGraph' config setting that triggers writing a
> commit-graph file after any non-trivial 'git gc' command. Defaults to
> false while the commit-graph feature matures. We specifically do not
> want to turn this on by default until the commit-graph feature is fully
s/turn this on/have this on/ I think.
> integrated with history-modifying features like shallow clones.
Two things.
First, shallow clones, replacement mechanims (git-replace) and grafts
are not "history-modifying" features; this name is in my opinion
reserved for history-rewriting features such as interactive rebase, the
`git filter-branch` feature or out-of-tree BFG Repo Cleaner or
reposurgeon tools. They alter the _view_ of history; they should be
IMVHO named "history-view-altering" features -- though I agree this is
mouthful.
Second, shouldn't we, as Martin Ågren said, warn about the issue in the
manpage for git-commit-graph?
>
> Signed-off-by: Derrick Stolee
> ---
> Documentation/config.txt | 6 ++
> Documentation/git-gc.txt | 4
> builtin/gc.c | 6 ++
> t/t5318-commit-graph.sh | 14 ++
> 4 files changed, 30 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 11f027194e..9a3abd87e7 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1553,6 +1553,12 @@ gc.autoDetach::
> Make `git gc --auto` return immediately and run in background
> if the system supports it. Default is true.
>
> +gc.commitGraph::
> + If true, then gc will rewrite the commit-graph file after any
> + change to the object database. If '--auto' is used, then the
> + commit-graph will not be updated unless the threshold is met.
What threshold? Ah, thresholds defined for `git gc --auto` (gc.auto,
gc.autoPackLimit, gc.logExpiry,...).
> + See linkgit:git-commit-graph[1] for details.
You missed declaring the default value for this config option.
> +
> gc.logExpiry::
> If the file gc.log exists, then `git gc --auto` won't run
> unless that file is more than 'gc.logExpiry' old. Default is
> diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
> index 571b5a7e3c..17dd654a59 100644
> --- a/Documentation/git-gc.txt
> +++ b/Documentation/git-gc.txt
> @@ -119,6 +119,10 @@ The optional configuration variable `gc.packRefs`
> determines if
> it within all non-bare repos or it can be set to a boolean value.
> This defaults to true.
>
> +The optional configuration variable 'gc.commitGraph' determines if
> +'git gc' runs 'git commit-graph write'. This can be set to a boolean
Should it be "runs" or "should run"?
> +value. This defaults to false.
Should it be '...' or `...`? Below we have `gc.aggresiveWindow`, above
we have 'gc.commitGraph', for example.
> +
> The optional configuration variable `gc.aggressiveWindow` controls how
> much time is spent optimizing the delta compression of the objects in
> the repository when the --aggressive option is specified. The larger
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 77fa720bd0..efd214a59f 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -20,6 +20,7 @@
> #include "argv-array.h"
> #include "commit.h"
> #include "packfile.h"
> +#include "commit-graph.h"
>
> #define FAILED_RUN "failed to run %s"
>
> @@ -34,6 +35,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_commit_graph = 0;
> static int detach_auto = 1;
> static timestamp_t gc_log_expire_time;
> static const char *gc_log_expire = "1.day.ago";
> @@ -121,6 +123,7 @@ static void gc_config(void)
> git_config_get_int("gc.aggressivedepth", _depth);
> git_config_get_int("gc.auto", _auto_threshold);
> git_config_get_int("gc.autopacklimit", _auto_pack_limit);
> + git_config_get_bool("gc.commitgraph", _commit_graph);
> git_config_get_bool("gc.autodetach", _auto);
> git_config_get_expiry("gc.pruneexpire", _expire);
> git_config_get_expiry("gc.worktreepruneexpire",
> _worktrees_expire);
> @@ -480,6 +483,9 @@ int cmd_gc(int argc, const char **argv, const char
> *prefix)
> if (pack_garbage.nr > 0)
> clean_pack_garbage();
>
> + if (gc_commit_graph)
> + write_commit_graph_reachable(get_object_directory(), 0);
> +
Nice.
Though now I wonder when appending should be used...
> if (auto_gc &&
Re: [PATCH v3 18/20] commit-graph: add '--reachable' option
Derrick Stolee writes:
> When writing commit-graph files, it can be convenient to ask for all
> reachable commits (starting at the ref set) in the resulting file. This
> is particularly helpful when writing to stdin is complicated, such as a
> future integration with 'git gc' which will call
> write_commit_graph_reachable() after performing cleanup of the object
> database.
Nice.
The last sentence of the commit message is a bit long, though, in my
opinion.
>
> Signed-off-by: Derrick Stolee
> ---
> Documentation/git-commit-graph.txt | 8 ++--
> builtin/commit-graph.c | 16
> commit-graph.c | 32
> commit-graph.h | 1 +
> t/t5318-commit-graph.sh| 10 ++
> 5 files changed, 61 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-commit-graph.txt
> b/Documentation/git-commit-graph.txt
> index a222cfab08..dececb79d7 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -38,12 +38,16 @@ Write a commit graph file based on the commits found in
> packfiles.
> +
> With the `--stdin-packs` option, generate the new commit graph by
> walking objects only in the specified pack-indexes. (Cannot be combined
> -with --stdin-commits.)
> +with `--stdin-commits` or `--reachable`.)
> +
> With the `--stdin-commits` option, generate the new commit graph by
> walking commits starting at the commits specified in stdin as a list
> of OIDs in hex, one OID per line. (Cannot be combined with
> ---stdin-packs.)
> +`--stdin-packs` or `--reachable`.)
> ++
> +With the `--reachable` option, generate the new commit graph by walking
> +commits starting at all refs. (Cannot be combined with `--stdin-commits`
> +or `--stdin-packs`.)
All right (though I wonder a bit about the restriction).
I think it might be a good idea to describe all of this in the usage
string for the 'git commit-graph write', instead of using ''
placeholder, that is instead of current:
'git commit-graph write' [--object-dir ]
use
'git commit-graph write' [--stdin-commits | --stdin-packs | --reachable]
[--append] [--object-dir ]
or something like that.
> +
> With the `--append` option, include all commits that are present in the
> existing commit-graph file.
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 0433dd6e20..20ce6437ae 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -9,7 +9,7 @@ static char const * const builtin_commit_graph_usage[] = {
> N_("git commit-graph [--object-dir ]"),
> N_("git commit-graph read [--object-dir ]"),
> N_("git commit-graph verify [--object-dir ]"),
> - N_("git commit-graph write [--object-dir ] [--append]
> [--stdin-packs|--stdin-commits]"),
> + N_("git commit-graph write [--object-dir ] [--append]
> [--reachable|--stdin-packs|--stdin-commits]"),
All right, very straightforward. I guess they are put in [almost]
alphabetical order, or is there some other reasoning behind the ordering
used (which is different from the one in the manpage)?
> NULL
> };
>
> @@ -24,12 +24,13 @@ static const char * const
> builtin_commit_graph_read_usage[] = {
> };
>
> static const char * const builtin_commit_graph_write_usage[] = {
> - N_("git commit-graph write [--object-dir ] [--append]
> [--stdin-packs|--stdin-commits]"),
> + N_("git commit-graph write [--object-dir ] [--append]
> [--reachable|--stdin-packs|--stdin-commits]"),
The same.
> NULL
> };
>
> static struct opts_commit_graph {
> const char *obj_dir;
> + int reachable;
> int stdin_packs;
> int stdin_commits;
> int append;
> @@ -130,6 +131,8 @@ static int graph_write(int argc, const char **argv)
> OPT_STRING(0, "object-dir", _dir,
> N_("dir"),
> N_("The object directory to store the graph")),
> + OPT_BOOL(0, "reachable", ,
> + N_("start walk at all refs")),
Errr... does '--no-reachable' makes sense? Because if I am right
currently it is supported, isn't it.
> OPT_BOOL(0, "stdin-packs", _packs,
> N_("scan pack-indexes listed by stdin for commits")),
> OPT_BOOL(0, "stdin-commits", _commits,
> @@ -143,11 +146,16 @@ static int graph_write(int argc, const char **argv)
>builtin_commit_graph_write_options,
>builtin_commit_graph_write_usage, 0);
>
> - if (opts.stdin_packs && opts.stdin_commits)
> - die(_("cannot use both --stdin-commits and --stdin-packs"));
> + if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)
Nice trick.
> + die(_("use at most one of --reachable, --stdin-commits, or
> --stdin-packs"));
It is a pity that parseopt API does not have direct support for mutually
exclusive
Re: [PATCH v3 17/20] fsck: verify commit-graph
Derrick Stolee writes:
> If core.commitGraph is true, verify the contents of the commit-graph
> during 'git fsck' using the 'git commit-graph verify' subcommand. Run
> this check on all alternates, as well.
All right, so we have one config variable to control the use of
serialized commit-graph feaature. Nice.
>
> We use a new process for two reasons:
>
> 1. The subcommand decouples the details of loading and verifying a
>commit-graph file from the other fsck details.
All right, I can agree with that.
On the other hand using subcommand makes debugging harder, though not in
this case (well separated functionality that can be easily called with a
standalone command to be debugged).
>
> 2. The commit-graph verification requires the commits to be loaded
>in a specific order to guarantee we parse from the commit-graph
>file for some objects and from the object database for others.
I don't quite understand this. Could you explain it in more detail?
>
> Signed-off-by: Derrick Stolee
> ---
> Documentation/git-fsck.txt | 3 +++
> builtin/fsck.c | 21 +
> t/t5318-commit-graph.sh| 8
> 3 files changed, 32 insertions(+)
>
> diff --git a/Documentation/git-fsck.txt b/Documentation/git-fsck.txt
> index b9f060e3b2..ab9a93fb9b 100644
> --- a/Documentation/git-fsck.txt
> +++ b/Documentation/git-fsck.txt
> @@ -110,6 +110,9 @@ Any corrupt objects you will have to find in backups or
> other archives
> (i.e., you can just remove them and do an 'rsync' with some other site in
> the hopes that somebody else has the object you have corrupted).
>
> +If core.commitGraph is true, the commit-graph file will also be inspected
Shouldn't we use `core.commitGraph` here?
> +using 'git commit-graph verify'. See linkgit:git-commit-graph[1].
> +
> Extracted Diagnostics
> -
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index ef78c6c00c..a6d5045b77 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -16,6 +16,7 @@
> #include "streaming.h"
> #include "decorate.h"
> #include "packfile.h"
> +#include "run-command.h"
>
> #define REACHABLE 0x0001
> #define SEEN 0x0002
> @@ -45,6 +46,7 @@ static int name_objects;
> #define ERROR_REACHABLE 02
> #define ERROR_PACK 04
> #define ERROR_REFS 010
> +#define ERROR_COMMIT_GRAPH 020
Minor nitpick and a sidenote: I wonder if it wouldn't be better to
either use hexadecimal constants, or use (1 << n) for all ERROR_*
preprocesor constants.
>
> static const char *describe_object(struct object *obj)
> {
> @@ -815,5 +817,24 @@ int cmd_fsck(int argc, const char **argv, const char
> *prefix)
> }
>
> check_connectivity();
> +
> + if (core_commit_graph) {
> + struct child_process commit_graph_verify = CHILD_PROCESS_INIT;
> + const char *verify_argv[] = { "commit-graph", "verify", NULL,
> NULL, NULL, NULL };
I see that NULL at index 2 and 3 (at 3rd and 4th place) are here for
"--object-dir" and , the last one is
terminator for that case, but what is next to last NULL (at 5th place)
for?
> + commit_graph_verify.argv = verify_argv;
> + commit_graph_verify.git_cmd = 1;
> +
> + if (run_command(_graph_verify))
> + errors_found |= ERROR_COMMIT_GRAPH;
> +
> + prepare_alt_odb();
> + for (alt = alt_odb_list; alt; alt = alt->next) {
> + verify_argv[2] = "--object-dir";
> + verify_argv[3] = alt->path;
> + if (run_command(_graph_verify))
> + errors_found |= ERROR_COMMIT_GRAPH;
> + }
> + }
For performance reasons it may be better to start those 'git
commit-graph verify' commands asynchronously earlier, so that they can
run in parallel / concurrently wth other checks, and wait for them and
get their error code at the end of git-fsck run.
But that is probably better left for a separate commit.
> +
> return errors_found;
> }
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 2680a2ebff..4941937163 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -394,4 +394,12 @@ test_expect_success 'detect invalid checksum hash' '
> "incorrect checksum"
> '
>
> +test_expect_success 'git fsck (checks commit-graph)' '
> + cd "$TRASH_DIRECTORY/full" &&
> + git fsck &&
> + corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
> + "incorrect checksum" &&
> + test_must_fail git fsck
> +'
All right; though the same caveats apply as with previous commit in
series. Perhaps it would be better to truncate commit-graph file, or
corrupt it in some 'random' place.
> +
> test_done
Best,
--
Jakub Narębski
Re: [PATCH v3 16/20] commit-graph: verify contents match checksum
Derrick Stolee writes:
> The commit-graph file ends with a SHA1 hash of the previous contents. If
> a commit-graph file has errors but the checksum hash is correct, then we
> know that the problem is a bug in Git and not simply file corruption
> after-the-fact.
>
> Compute the checksum right away so it is the first error that appears,
> and make the message translatable since this error can be "corrected" by
> a user by simply deleting the file and recomputing. The rest of the
> errors are useful only to developers.
Should we then provide --quiet / --verbose options, so that ordinary
user is not flooded with error messages meant for power users and Git
developers, then?
>
> Be sure to continue checking the rest of the file data if the checksum
> is wrong. This is important for our tests, as we break the checksum as
> we modify bytes of the commit-graph file.
Well, we could have used sha1sum program, or test-sha1 helper to fix the
checksum after corrupting the commit-graph file...
>
> Signed-off-by: Derrick Stolee
> ---
> commit-graph.c | 16 ++--
> t/t5318-commit-graph.sh | 6 ++
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index d2b291aca2..a33600c584 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -841,6 +841,7 @@ void write_commit_graph(const char *obj_dir,
> oids.nr = 0;
> }
>
> +#define VERIFY_COMMIT_GRAPH_ERROR_HASH 2
> static int verify_commit_graph_error;
>
> static void graph_report(const char *fmt, ...)
> @@ -860,7 +861,9 @@ static void graph_report(const char *fmt, ...)
> int verify_commit_graph(struct commit_graph *g)
> {
> uint32_t i, cur_fanout_pos = 0;
> - struct object_id prev_oid, cur_oid;
> + struct object_id prev_oid, cur_oid, checksum;
> + struct hashfile *f;
> + int devnull;
>
> if (!g) {
> graph_report("no commit-graph file loaded");
> @@ -879,6 +882,15 @@ int verify_commit_graph(struct commit_graph *g)
> if (verify_commit_graph_error)
> return verify_commit_graph_error;
>
> + devnull = open("/dev/null", O_WRONLY);
> + f = hashfd(devnull, NULL);
> + hashwrite(f, g->data, g->data_len - g->hash_len);
> + finalize_hashfile(f, checksum.hash, CSUM_CLOSE);
> + if (hashcmp(checksum.hash, g->data + g->data_len - g->hash_len)) {
> + graph_report(_("the commit-graph file has incorrect checksum
> and is likely corrupt"));
> + verify_commit_graph_error = VERIFY_COMMIT_GRAPH_ERROR_HASH;
> + }
Is it the best way of calculating the SHA-1 checksum that out internal
APIs provide? Is it how SHA-1 checksum is calculated and checked for
packfiles?
> +
> for (i = 0; i < g->num_commits; i++) {
> struct commit *graph_commit;
>
> @@ -916,7 +928,7 @@ int verify_commit_graph(struct commit_graph *g)
> cur_fanout_pos++;
> }
>
> - if (verify_commit_graph_error)
> + if (verify_commit_graph_error & ~VERIFY_COMMIT_GRAPH_ERROR_HASH)
> return verify_commit_graph_error;
So if we detected that checksum do not match, but we have not found an
error, we say that it is all right?
>
> for (i = 0; i < g->num_commits; i++) {
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 240aef6add..2680a2ebff 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -279,6 +279,7 @@ GRAPH_COMMIT_DATA_WIDTH=`expr $HASH_LEN + 16`
> GRAPH_OCTOPUS_DATA_OFFSET=`expr $GRAPH_COMMIT_DATA_OFFSET + \
> $GRAPH_COMMIT_DATA_WIDTH \* $NUM_COMMITS`
> GRAPH_BYTE_OCTOPUS=`expr $GRAPH_OCTOPUS_DATA_OFFSET + 4`
> +GRAPH_BYTE_FOOTER=`expr $GRAPH_OCTOPUS_DATA_OFFSET + 4 \* $NUM_OCTOPUS_EDGES`
>
> # usage: corrupt_graph_and_verify
> # Manipulates the commit-graph file at the position
> @@ -388,4 +389,9 @@ test_expect_success 'detect incorrect parent for octopus
> merge' '
> "invalid parent"
> '
>
> +test_expect_success 'detect invalid checksum hash' '
> + corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
> + "incorrect checksum"
This would not work under GETTEXT_POISON, as the message is marked as
translatable, but corrupt_graph_and_verify uses 'grep' and not
'test_i18grep' from t/test-lib-functions.sh
> +'
If it is pure checksum corruption, wouldn't this fail because it is not
a failure (exit code is 0)?
> +
> test_done
Re
I am Ms.Ella Golan, I am the Executive Vice President Banking Division with FIRST INTERNATIONAL BANK OF ISRAEL LTD (FIBI). I am getting in touch with you regarding an extremely important and urgent matter. If you would oblige me the opportunity, I shall provide you with details upon your response. Faithfully, Ms.Ella Golan
Re: [PATCH v3 15/20] commit-graph: test for corrupted octopus edge
Derrick Stolee writes: > The commit-graph file has an extra chunk to store the parent int-ids for > parents beyond the first parent for octopus merges. Our test repo has a > single octopus merge that we can manipulate to demonstrate the 'verify' > subcommand detects incorrect values in that chunk. If I understand it correctly the above means that our _reading_ code checks for validity (which then 'git commit-graph verify' uses), just there were not any tests for that. > > Signed-off-by: Derrick Stolee > --- > t/t5318-commit-graph.sh | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh > index 58adb8246d..240aef6add 100755 > --- a/t/t5318-commit-graph.sh > +++ b/t/t5318-commit-graph.sh > @@ -248,6 +248,7 @@ test_expect_success 'git commit-graph verify' ' > ' > > NUM_COMMITS=9 > +NUM_OCTOPUS_EDGES=2 > HASH_LEN=20 > GRAPH_BYTE_VERSION=4 > GRAPH_BYTE_HASH=5 > @@ -274,6 +275,10 @@ GRAPH_BYTE_COMMIT_EXTRA_PARENT=`expr > $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 4` > GRAPH_BYTE_COMMIT_WRONG_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + > 3` > GRAPH_BYTE_COMMIT_GENERATION=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 8` > GRAPH_BYTE_COMMIT_DATE=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 12` > +GRAPH_COMMIT_DATA_WIDTH=`expr $HASH_LEN + 16` > +GRAPH_OCTOPUS_DATA_OFFSET=`expr $GRAPH_COMMIT_DATA_OFFSET + \ > + $GRAPH_COMMIT_DATA_WIDTH \* $NUM_COMMITS` > +GRAPH_BYTE_OCTOPUS=`expr $GRAPH_OCTOPUS_DATA_OFFSET + 4` > > # usage: corrupt_graph_and_verify > # Manipulates the commit-graph file at the position > @@ -378,4 +383,9 @@ test_expect_success 'detect incorrect commit date' ' > "commit date" > ' > > +test_expect_success 'detect incorrect parent for octopus merge' ' > + corrupt_graph_and_verify $GRAPH_BYTE_OCTOPUS "\01" \ > + "invalid parent" > +' So we change the int-id to non-existing commit, and check that commit-graph code checks for that. What about the case when there are octopus merges, but no EDGE chunk (which I think we can emulate by changing / corrupting number of chunks)? What about the case where int-id of edge in EDGE chunk is correct, that is points to a valid commit, but does not agree with what is in the object database (what parents octopus merge has in reality)? Do we detect the situation where the second parent value in the commit data stores an array position within a Large Edge chunk, but we do not reach a value with the most-significant bit on when reaching the end of Large Edge chunk? > + > test_done
Re: [PATCH v3 14/20] commit-graph: verify commit date
Derrick Stolee writes:
Nice and simple. The only possible question may be the ordering of
patches in the series, namely whether this change should be before or
after test checking generation numbers.
> Signed-off-by: Derrick Stolee
> ---
> commit-graph.c | 6 ++
> t/t5318-commit-graph.sh | 6 ++
> 2 files changed, 12 insertions(+)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index ead92460c1..d2b291aca2 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -981,6 +981,12 @@ int verify_commit_graph(struct commit_graph *g)
>oid_to_hex(_oid),
>graph_commit->generation,
>max_generation + 1);
> +
> + if (graph_commit->date != odb_commit->date)
> + graph_report("commit date for commit %s in commit-graph
> is %"PRItime" != %"PRItime,
> + oid_to_hex(_oid),
> + graph_commit->date,
> + odb_commit->date);
All right. Very straightforward, good.
> }
>
> return verify_commit_graph_error;
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 673b0d37d5..58adb8246d 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -273,6 +273,7 @@ GRAPH_BYTE_COMMIT_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET
> + $HASH_LEN`
> GRAPH_BYTE_COMMIT_EXTRA_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN +
> 4`
> GRAPH_BYTE_COMMIT_WRONG_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN +
> 3`
> GRAPH_BYTE_COMMIT_GENERATION=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 8`
> +GRAPH_BYTE_COMMIT_DATE=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 12`
>
> # usage: corrupt_graph_and_verify
> # Manipulates the commit-graph file at the position
> @@ -372,4 +373,9 @@ test_expect_success 'detect incorrect generation number' '
> "generation"
> '
>
> +test_expect_success 'detect incorrect commit date' '
> + corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_DATE "\01" \
> + "commit date"
> +'
All right.
> +
> test_done
Re: [PATCH] t9104: kosherly remove remote refs
On Fri, Jun 1, 2018 at 7:08 AM, Christian Couder
wrote:
> test_expect_success "multi-fetch works off a 'clean' repository" '
> - rm -rf "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" &&
> + rm -rf "$GIT_DIR/svn" &&
> + git for-each-ref --format="option no-deref%0adelete %(refname)"
> refs/remotes |
> + git update-ref --stdin &&
Is that "option no-deref" really necessary when deleting refs? Does
it have any effect at all?
The synopsis in 'git update-ref's manpage indicates that '--no-deref'
is only applicable when updating a ref, but not when deleting one;
though the usage shown by 'git update-ref -h' doesn't indicate this.
Anyway, it appears that when deleting refs no symref dereferencing is
performed and '--no-deref' is simply ignored:
$ git branch one
$ git branch two
$ git symbolic-ref ONE refs/heads/one
$ git symbolic-ref TWO refs/heads/two
$ cat .git/{ONE,TWO}
ref: refs/heads/one
ref: refs/heads/two
$ git update-ref -d ONE
$ git update-ref --no-deref -d TWO
$ cat .git/{ONE,TWO}
cat: .git/ONE: No such file or directory
cat: .git/TWO: No such file or directory
$ git for-each-ref
95c5b8654fd75df13ed29f43cff52287414c3877 commit refs/heads/master
95c5b8654fd75df13ed29f43cff52287414c3877 commit refs/heads/one
95c5b8654fd75df13ed29f43cff52287414c3877 commit refs/heads/two
Re: [PATCH v3 13/20] commit-graph: verify generation number
Derrick Stolee writes:
> While iterating through the commit parents, perform the generation
> number calculation and compare against the value stored in the
> commit-graph.
All right, that's good.
What about commit-graph files that have GENERATION_NUMBER_ZERO for all
its commits (because we verify single commit-graph file only, there
wouldn't be GENERATION_NUMBER_ZERO mixed with non-zero generation
numbers)?
Unless we can assume that no commit-graph file in the wild would have
GENERATION_NUMBER_ZERO.
>
> The tests demonstrate that having a different set of parents affects
> the generation number calculation, and this value propagates to
> descendants. Hence, we drop the single-line condition on the output.
I don't understand what part of changes this paragraph of the commit
message refers to.
Anyway, changing parents may not lead to changed generation numbers;
take for example commit with single parent, which we change to other
commit with the same generation number.
>
> Signed-off-by: Derrick Stolee
> ---
> commit-graph.c | 18 ++
> t/t5318-commit-graph.sh | 6 ++
Sidenote: I have just realized that it may be better to put
validation-related tests into different test file.
> 2 files changed, 24 insertions(+)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index fff22dc0c3..ead92460c1 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -922,6 +922,7 @@ int verify_commit_graph(struct commit_graph *g)
> for (i = 0; i < g->num_commits; i++) {
> struct commit *graph_commit, *odb_commit;
> struct commit_list *graph_parents, *odb_parents;
> + uint32_t max_generation = 0;
>
> hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
>
> @@ -956,6 +957,9 @@ int verify_commit_graph(struct commit_graph *g)
>
> oid_to_hex(_parents->item->object.oid),
>
> oid_to_hex(_parents->item->object.oid));
>
> + if (graph_parents->item->generation > max_generation)
> + max_generation =
> graph_parents->item->generation;
> +
All right, that calculates the maximum of generation number of commit
parents.
> graph_parents = graph_parents->next;
> odb_parents = odb_parents->next;
> }
> @@ -963,6 +967,20 @@ int verify_commit_graph(struct commit_graph *g)
> if (odb_parents != NULL)
> graph_report("commit-graph parent list for commit %s
> terminates early",
>oid_to_hex(_oid));
> +
> + /*
> + * If one of our parents has generation GENERATION_NUMBER_MAX,
> then
> + * our generation is also GENERATION_NUMBER_MAX. Decrement to
> avoid
> + * extra logic in the following condition.
> + */
Nice trick.
> + if (max_generation == GENERATION_NUMBER_MAX)
> + max_generation--;
What about GENERATION_NUMBER_ZERO?
> +
> + if (graph_commit->generation != max_generation + 1)
> + graph_report("commit-graph generation for commit %s is
> %u != %u",
> + oid_to_hex(_oid),
> + graph_commit->generation,
> + max_generation + 1);
I think we should also check that generation numbers do not exceed
GENERATION_NUMBER_MAX... unless it is already taken care of?
> }
>
> return verify_commit_graph_error;
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 12f0d7f54d..673b0d37d5 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -272,6 +272,7 @@ GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSET
> GRAPH_BYTE_COMMIT_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN`
> GRAPH_BYTE_COMMIT_EXTRA_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN +
> 4`
> GRAPH_BYTE_COMMIT_WRONG_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN +
> 3`
> +GRAPH_BYTE_COMMIT_GENERATION=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 8`
>
> # usage: corrupt_graph_and_verify
> # Manipulates the commit-graph file at the position
> @@ -366,4 +367,9 @@ test_expect_success 'detect incorrect tree OID' '
> "commit-graph parent for"
> '
>
> +test_expect_success 'detect incorrect generation number' '
> + corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_GENERATION "\01" \
I assume that you have checked that it actually corrupts generation
number (without affecting commit date).
> + "generation"
A very minor nitpick: Not "generation for commit"?
> +'
> +
> test_done
[PATCH v6 1/8] checkout tests: index should be clean after dwim checkout
Assert that whenever there's a DWIM checkout that the index should be
clean afterwards, in addition to the correct branch being checked-out.
The way the DWIM checkout code in checkout.[ch] works is by looping
over all remotes, and for each remote trying to find if a given
reference name only exists on that remote, or if it exists anywhere
else.
This is done by starting out with a `unique = 1` tracking variable in
a struct shared by the entire loop, which will get set to `0` if the
data reference is not unique.
Thus if we find a match we know the dst_oid member of
tracking_name_data must be correct, since it's associated with the
only reference on the only remote that could have matched our query.
But if there was ever a mismatch there for some reason we might end up
with the correct branch checked out, but at the wrong oid, which would
show whatever the difference between the two staged in the
index (checkout branch A, stage changes from the state of branch B).
So let's amend the tests (mostly added in) 399e4a1c56 ("t2024: Add
tests verifying current DWIM behavior of 'git checkout '",
2013-04-21) to always assert that "status" is clean after we run
"checkout", that's being done with "-uno" because there's going to be
some untracked files related to the test itself which we don't care
about.
In all these tests (DWIM or otherwise) we start with a clean index, so
these tests are asserting that that's still the case after the
"checkout", failed or otherwise.
Then if we ever run into this sort of regression, either in the
existing code or with a new feature, we'll know.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t2024-checkout-dwim.sh | 29 +
1 file changed, 29 insertions(+)
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 3e5ac81bd2..ed32828105 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -23,6 +23,12 @@ test_branch_upstream () {
test_cmp expect.upstream actual.upstream
}
+status_uno_is_clean() {
+ >status.expect &&
+ git status -uno --porcelain >status.actual &&
+ test_cmp status.expect status.actual
+}
+
test_expect_success 'setup' '
test_commit my_master &&
git init repo_a &&
@@ -55,6 +61,7 @@ test_expect_success 'checkout of non-existing branch fails' '
test_might_fail git branch -D xyzzy &&
test_must_fail git checkout xyzzy &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/xyzzy &&
test_branch master
'
@@ -64,6 +71,7 @@ test_expect_success 'checkout of branch from multiple remotes
fails #1' '
test_might_fail git branch -D foo &&
test_must_fail git checkout foo &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/foo &&
test_branch master
'
@@ -73,6 +81,7 @@ test_expect_success 'checkout of branch from a single remote
succeeds #1' '
test_might_fail git branch -D bar &&
git checkout bar &&
+ status_uno_is_clean &&
test_branch bar &&
test_cmp_rev remotes/repo_a/bar HEAD &&
test_branch_upstream bar repo_a bar
@@ -83,6 +92,7 @@ test_expect_success 'checkout of branch from a single remote
succeeds #2' '
test_might_fail git branch -D baz &&
git checkout baz &&
+ status_uno_is_clean &&
test_branch baz &&
test_cmp_rev remotes/other_b/baz HEAD &&
test_branch_upstream baz repo_b baz
@@ -90,6 +100,7 @@ test_expect_success 'checkout of branch from a single remote
succeeds #2' '
test_expect_success '--no-guess suppresses branch auto-vivification' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D bar &&
test_must_fail git checkout --no-guess bar &&
@@ -99,6 +110,7 @@ test_expect_success '--no-guess suppresses branch
auto-vivification' '
test_expect_success 'setup more remotes with unconventional refspecs' '
git checkout -B master &&
+ status_uno_is_clean &&
git init repo_c &&
(
cd repo_c &&
@@ -128,27 +140,33 @@ test_expect_success 'setup more remotes with
unconventional refspecs' '
test_expect_success 'checkout of branch from multiple remotes fails #2' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D bar &&
test_must_fail git checkout bar &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/bar &&
test_branch master
'
test_expect_success 'checkout of branch from multiple remotes fails #3' '
git checkout -B master &&
+ status_uno_is_clean &&
test_might_fail git branch -D baz &&
test_must_fail git checkout baz &&
+ status_uno_is_clean &&
test_must_fail git rev-parse --verify refs/heads/baz &&
test_branch master
'
test_expect_success 'checkout of branch from a single
[PATCH v6 6/8] builtin/checkout.c: use "ret" variable for return
There is no point in doing this right now, but in later change the
"ret" variable will be inspected. This change makes that meaningful
change smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
builtin/checkout.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 72457fb7d5..8c93c55cbc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1265,8 +1265,10 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
}
UNLEAK(opts);
- if (opts.patch_mode || opts.pathspec.nr)
- return checkout_paths(, new_branch_info.name);
- else
+ if (opts.patch_mode || opts.pathspec.nr) {
+ int ret = checkout_paths(, new_branch_info.name);
+ return ret;
+ } else {
return checkout_branch(, _branch_info);
+ }
}
--
2.17.0.290.gded63e768a
[PATCH v6 7/8] checkout: add advice for ambiguous "checkout "
As the "checkout" documentation describes:
If is not found but there does exist a tracking branch in
exactly one remote (call it ) with a matching name, treat
as equivalent to [...] /
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/config.txt | 7 +++
advice.c | 2 ++
advice.h | 1 +
builtin/checkout.c | 13 +
t/t2024-checkout-dwim.sh | 14 ++
5 files changed, 37 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a9..dfc0413a84 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -344,6 +344,13 @@ advice.*::
Advice shown when you used linkgit:git-checkout[1] to
move to the detach HEAD state, to instruct how to create
a local branch after the fact.
+ checkoutAmbiguousRemoteBranchName::
+ Advice shown when the argument to
+ linkgit:git-checkout[1] ambiguously resolves to a
+ remote tracking branch on more than one remote in
+ situations where an unambiguous argument would have
+ otherwise caused a remote-tracking branch to be
+ checked out.
amWorkDir::
Advice that shows the location of the patch file when
linkgit:git-am[1] fails to apply it.
diff --git a/advice.c b/advice.c
index 370a56d054..75e7dede90 100644
--- a/advice.c
+++ b/advice.c
@@ -21,6 +21,7 @@ int advice_add_embedded_repo = 1;
int advice_ignored_hook = 1;
int advice_waiting_for_editor = 1;
int advice_graft_file_deprecated = 1;
+int advice_checkout_ambiguous_remote_branch_name = 1;
static int advice_use_color = -1;
static char advice_colors[][COLOR_MAXLEN] = {
@@ -72,6 +73,7 @@ static struct {
{ "ignoredhook", _ignored_hook },
{ "waitingforeditor", _waiting_for_editor },
{ "graftfiledeprecated", _graft_file_deprecated },
+ { "checkoutambiguousremotebranchname",
_checkout_ambiguous_remote_branch_name },
/* make this an alias for backward compatibility */
{ "pushnonfastforward", _push_update_rejected }
diff --git a/advice.h b/advice.h
index 9f5064e82a..4d11d51d43 100644
--- a/advice.h
+++ b/advice.h
@@ -22,6 +22,7 @@ extern int advice_add_embedded_repo;
extern int advice_ignored_hook;
extern int advice_waiting_for_editor;
extern int advice_graft_file_deprecated;
+extern int advice_checkout_ambiguous_remote_branch_name;
int git_default_advice_config(const char *var, const char *value);
__attribute__((format (printf, 1, 2)))
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8c93c55cbc..baa027455a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -22,6 +22,7 @@
#include "resolve-undo.h"
#include "submodule-config.h"
#include "submodule.h"
+#include "advice.h"
static const char * const checkout_usage[] = {
N_("git checkout [] "),
@@ -1267,6 +1268,18 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
UNLEAK(opts);
if (opts.patch_mode || opts.pathspec.nr) {
int ret = checkout_paths(, new_branch_info.name);
+ if (ret && dwim_remotes_matched > 1 &&
+ advice_checkout_ambiguous_remote_branch_name)
+ advise(_("'%s' matched more than one remote tracking
branch.\n"
+"We found %d remotes with a reference that
matched. So we fell back\n"
+"on trying to resolve the argument as a path,
but failed there too!\n"
+"\n"
+"If you meant to check out a remote tracking
branch on, e.g. 'origin',\n"
+"you can do so by fully qualifying the name
with the --track option:\n"
+"\n"
+"git checkout --track origin/"),
+ argv[0],
+ dwim_remotes_matched);
return ret;
} else {
return checkout_branch(, _branch_info);
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index ed32828105..fef263a858 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -76,6 +76,20 @@ test_expect_success 'checkout of branch from multiple
remotes fails #1' '
test_branch master
'
+test_expect_success 'checkout of branch from multiple remotes fails with
advice' '
+ git checkout -B master &&
+ test_might_fail git branch -D foo &&
+ test_must_fail git checkout foo 2>stderr &&
+ test_branch master &&
+ status_uno_is_clean &&
+ test_i18ngrep "^hint: " stderr &&
+ test_must_fail git -c advice.checkoutAmbiguousRemoteBranchName=false \
+ checkout foo 2>stderr &&
+ test_branch master &&
+ status_uno_is_clean &&
+ test_i18ngrep ! "^hint: " stderr
+'
+
[PATCH v6 3/8] checkout.c: introduce an *_INIT macro
Add an *_INIT macro for the tracking_name_data similar to what exists
elsewhere in the codebase, e.g. OID_ARRAY_INIT in sha1-array.h. This
will make it more idiomatic in later changes to add more fields to the
struct & its initialization macro.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
checkout.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/checkout.c b/checkout.c
index bdefc888ba..80e430cda8 100644
--- a/checkout.c
+++ b/checkout.c
@@ -10,6 +10,8 @@ struct tracking_name_data {
int unique;
};
+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 1 }
+
static int check_tracking_name(struct remote *remote, void *cb_data)
{
struct tracking_name_data *cb = cb_data;
@@ -32,7 +34,7 @@ static int check_tracking_name(struct remote *remote, void
*cb_data)
const char *unique_tracking_name(const char *name, struct object_id *oid)
{
- struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
+ struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, _data);
--
2.17.0.290.gded63e768a
[PATCH v6 8/8] checkout & worktree: introduce checkout.defaultRemote
Introduce a checkout.defaultRemote setting which can be used to
designate a remote to prefer (via checkout.defaultRemote=origin) when
running e.g. "git checkout master" to mean origin/master, even though
there's other remotes that have the "master" branch.
I want this because it's very handy to use this workflow to checkout a
repository and create a topic branch, then get back to a "master" as
retrieved from upstream:
(
cd /tmp &&
rm -rf tbdiff &&
git clone g...@github.com:trast/tbdiff.git &&
cd tbdiff &&
git branch -m topic &&
git checkout master
)
That will output:
Branch 'master' set up to track remote branch 'master' from 'origin'.
Switched to a new branch 'master'
But as soon as a new remote is added (e.g. just to inspect something
from someone else) the DWIMery goes away:
(
cd /tmp &&
rm -rf tbdiff &&
git clone g...@github.com:trast/tbdiff.git &&
cd tbdiff &&
git branch -m topic &&
git remote add avar g...@github.com:avar/tbdiff.git &&
git fetch avar &&
git checkout master
)
Will output (without the advice output added earlier in this series):
error: pathspec 'master' did not match any file(s) known to git.
The new checkout.defaultRemote config allows me to say that whenever
that ambiguity comes up I'd like to prefer "origin", and it'll still
work as though the only remote I had was "origin".
Also adjust the advice.checkoutAmbiguousRemoteBranchName message to
mention this new config setting to the user, the full output on my
git.git is now (the last paragraph is new):
$ ./git --exec-path=$PWD checkout master
error: pathspec 'master' did not match any file(s) known to git.
hint: The argument 'master' matched more than one remote tracking branch.
hint: We found 26 remotes with a reference that matched. So we fell back
hint: on trying to resolve the argument as a path, but failed there too!
hint:
hint: If you meant to check out a remote tracking branch on e.g. 'origin'
hint: you can do so by fully-qualifying the name with the --track option:
hint:
hint: git checkout --track origin/
hint:
hint: If you'd like to always have checkouts of an ambiguous prefer
hint: one remote, e.g. the 'origin' remote, consider setting
hint: checkout.defaultRemote=origin in your config.
I considered splitting this into checkout.defaultRemote and
worktree.defaultRemote, but it's probably less confusing to break our
own rules that anything shared between config should live in core.*
than have two config settings, and I couldn't come up with a short
name under core.* that made sense (core.defaultRemoteForCheckout?).
See also 70c9ac2f19 ("DWIM "git checkout frotz" to "git checkout -b
frotz origin/frotz"", 2009-10-18) which introduced this DWIM feature
to begin with, and 4e85333197 ("worktree: make add
dwim", 2017-11-26) which added it to git-worktree.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/config.txt | 21 -
Documentation/git-checkout.txt | 9 +
Documentation/git-worktree.txt | 9 +
builtin/checkout.c | 12 +---
checkout.c | 26 --
t/t2024-checkout-dwim.sh | 18 +-
t/t2025-worktree-add.sh| 21 +
7 files changed, 109 insertions(+), 7 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index dfc0413a84..aef2769211 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -350,7 +350,10 @@ advice.*::
remote tracking branch on more than one remote in
situations where an unambiguous argument would have
otherwise caused a remote-tracking branch to be
- checked out.
+ checked out. See the `checkout.defaultRemote`
+ configuration variable for how to set a given remote
+ to used by default in some situations where this
+ advice would be printed.
amWorkDir::
Advice that shows the location of the patch file when
linkgit:git-am[1] fails to apply it.
@@ -1105,6 +1108,22 @@ browser..path::
browse HTML help (see `-w` option in linkgit:git-help[1]) or a
working repository in gitweb (see linkgit:git-instaweb[1]).
+checkout.defaultRemote::
+ When you run 'git checkout ' and only have one
+ remote, it may implicitly fall back on checking out and
+ tracking e.g. 'origin/'. This stops working as soon
+ as you have more than one remote with a ''
+ reference. This setting allows for setting the name of a
+ preferred remote that should always win when it comes to
+ disambiguation. The typical use-case is to set this to
+ `origin`.
++
+Currently this is used by linkgit:git-checkout[1] when 'git checkout
[PATCH v6 2/8] checkout.h: wrap the arguments to unique_tracking_name()
The line was too long already, and will be longer still when a later change adds another argument. Signed-off-by: Ævar Arnfjörð Bjarmason --- checkout.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/checkout.h b/checkout.h index 9980711179..4cd4cd1c23 100644 --- a/checkout.h +++ b/checkout.h @@ -8,6 +8,7 @@ * tracking branch. Return the name of the remote if such a branch * exists, NULL otherwise. */ -extern const char *unique_tracking_name(const char *name, struct object_id *oid); +extern const char *unique_tracking_name(const char *name, + struct object_id *oid); #endif /* CHECKOUT_H */ -- 2.17.0.290.gded63e768a
[PATCH v6 5/8] checkout: pass the "num_matches" up to callers
Pass the previously added "num_matches" struct value up to the callers
of unique_tracking_name(). This will allow callers to optionally print
better error messages in a later change.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
builtin/checkout.c | 10 +++---
builtin/worktree.c | 4 ++--
checkout.c | 5 -
checkout.h | 3 ++-
4 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2e1d2376d2..72457fb7d5 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -878,7 +878,8 @@ static int parse_branchname_arg(int argc, const char **argv,
int dwim_new_local_branch_ok,
struct branch_info *new_branch_info,
struct checkout_opts *opts,
- struct object_id *rev)
+ struct object_id *rev,
+ int *dwim_remotes_matched)
{
struct tree **source_tree = >source_tree;
const char **new_branch = >new_branch;
@@ -972,7 +973,8 @@ static int parse_branchname_arg(int argc, const char **argv,
recover_with_dwim = 0;
if (recover_with_dwim) {
- const char *remote = unique_tracking_name(arg, rev);
+ const char *remote = unique_tracking_name(arg, rev,
+
dwim_remotes_matched);
if (remote) {
*new_branch = arg;
arg = remote;
@@ -1109,6 +,7 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
struct branch_info new_branch_info;
char *conflict_style = NULL;
int dwim_new_local_branch = 1;
+ int dwim_remotes_matched = 0;
struct option options[] = {
OPT__QUIET(, N_("suppress progress reporting")),
OPT_STRING('b', NULL, _branch, N_("branch"),
@@ -1219,7 +1222,8 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
opts.track == BRANCH_TRACK_UNSPECIFIED &&
!opts.new_branch;
int n = parse_branchname_arg(argc, argv, dwim_ok,
-_branch_info, , );
+_branch_info, , ,
+_remotes_matched);
argv += n;
argc -= n;
}
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5c7d2bb180..a763dbdccb 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -412,7 +412,7 @@ static const char *dwim_branch(const char *path, const char
**new_branch)
if (guess_remote) {
struct object_id oid;
const char *remote =
- unique_tracking_name(*new_branch, );
+ unique_tracking_name(*new_branch, , NULL);
return remote;
}
return NULL;
@@ -484,7 +484,7 @@ static int add(int ac, const char **av, const char *prefix)
commit = lookup_commit_reference_by_name(branch);
if (!commit) {
- remote = unique_tracking_name(branch, );
+ remote = unique_tracking_name(branch, , NULL);
if (remote) {
new_branch = branch;
branch = remote;
diff --git a/checkout.c b/checkout.c
index 7662a39a62..ee3a7e9c05 100644
--- a/checkout.c
+++ b/checkout.c
@@ -32,12 +32,15 @@ static int check_tracking_name(struct remote *remote, void
*cb_data)
return 0;
}
-const char *unique_tracking_name(const char *name, struct object_id *oid)
+const char *unique_tracking_name(const char *name, struct object_id *oid,
+int *dwim_remotes_matched)
{
struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, _data);
+ if (dwim_remotes_matched)
+ *dwim_remotes_matched = cb_data.num_matches;
free(cb_data.src_ref);
if (cb_data.num_matches == 1)
return cb_data.dst_ref;
diff --git a/checkout.h b/checkout.h
index 4cd4cd1c23..6b2073310c 100644
--- a/checkout.h
+++ b/checkout.h
@@ -9,6 +9,7 @@
* exists, NULL otherwise.
*/
extern const char *unique_tracking_name(const char *name,
- struct object_id *oid);
+ struct object_id *oid,
+ int *dwim_remotes_matched);
#endif /* CHECKOUT_H */
--
2.17.0.290.gded63e768a
[PATCH v6 4/8] checkout.c]: change "unique" member to "num_matches"
Internally track how many matches we find in the check_tracking_name()
callback. Nothing uses this now, but it will be made use of in a later
change.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
checkout.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/checkout.c b/checkout.c
index 80e430cda8..7662a39a62 100644
--- a/checkout.c
+++ b/checkout.c
@@ -7,10 +7,10 @@ struct tracking_name_data {
/* const */ char *src_ref;
char *dst_ref;
struct object_id *dst_oid;
- int unique;
+ int num_matches;
};
-#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 1 }
+#define TRACKING_NAME_DATA_INIT { NULL, NULL, NULL, 0 }
static int check_tracking_name(struct remote *remote, void *cb_data)
{
@@ -23,9 +23,9 @@ static int check_tracking_name(struct remote *remote, void
*cb_data)
free(query.dst);
return 0;
}
+ cb->num_matches++;
if (cb->dst_ref) {
free(query.dst);
- cb->unique = 0;
return 0;
}
cb->dst_ref = query.dst;
@@ -39,7 +39,7 @@ const char *unique_tracking_name(const char *name, struct
object_id *oid)
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, _data);
free(cb_data.src_ref);
- if (cb_data.unique)
+ if (cb_data.num_matches == 1)
return cb_data.dst_ref;
free(cb_data.dst_ref);
return NULL;
--
2.17.0.290.gded63e768a
[PATCH v6 0/8] ambiguous checkout UI & checkout.defaultRemote
Typo & grammar fixes suggested by Eric Sunshine. tbdiff from v5:
1: ab4529d9f5 = 1: ab4529d9f5 checkout tests: index should be clean after
dwim checkout
2: c8bbece403 = 2: c8bbece403 checkout.h: wrap the arguments to
unique_tracking_name()
3: 4fc5ab27fa ! 3: 881fe63f4f checkout.c: introduce an *_INIT macro
@@ -1,6 +1,6 @@
Author: Ævar Arnfjörð Bjarmason
-checkout.[ch]: introduce an *_INIT macro
+checkout.c: introduce an *_INIT macro
Add an *_INIT macro for the tracking_name_data similar to what
exists
elsewhere in the codebase, e.g. OID_ARRAY_INIT in sha1-array.h.
This
4: fbce6df584 ! 4: 72ddaeddd3 checkout.c]: change "unique" member to
"num_matches"
@@ -1,6 +1,6 @@
Author: Ævar Arnfjörð Bjarmason
-checkout.[ch]: change "unique" member to "num_matches"
+checkout.c]: change "unique" member to "num_matches"
Internally track how many matches we find in the
check_tracking_name()
callback. Nothing uses this now, but it will be made use of in a
later
5: 6e016d43d7 = 5: 5e8c82680b checkout: pass the "num_matches" up to callers
6: 07b11b133d = 6: 07e667f80a builtin/checkout.c: use "ret" variable for
return
7: 97e84f6e1c ! 7: 0a148182e6 checkout: add advice for ambiguous "checkout
"
@@ -8,9 +8,9 @@
exactly one remote (call it ) with a matching name,
treat
as equivalent to [...] /
@@ -97,12 +97,12 @@
int ret = checkout_paths(, new_branch_info.name);
+ if (ret && dwim_remotes_matched > 1 &&
+ advice_checkout_ambiguous_remote_branch_name)
-+ advise(_("The argument '%s' matched more than
one remote tracking branch.\n"
++ advise(_("'%s' matched more than one remote
tracking branch.\n"
+ "We found %d remotes with a reference
that matched. So we fell back\n"
+ "on trying to resolve the argument as
a path, but failed there too!\n"
+ "\n"
-+ "If you meant to check out a remote
tracking branch on e.g. 'origin'\n"
-+ "you can do so by fully-qualifying the
name with the --track option:\n"
++ "If you meant to check out a remote
tracking branch on, e.g. 'origin',\n"
++ "you can do so by fully qualifying the
name with the --track option:\n"
+ "\n"
+ "git checkout --track
origin/"),
+ argv[0],
8: a5cc070ebf ! 8: f3a52a26a2 checkout & worktree: introduce
checkout.defaultRemote
@@ -175,8 +175,8 @@
* (c) Otherwise, if "--" is present, treat it like case (1).
*
@@
-"If you meant to check out a remote
tracking branch on e.g. 'origin'\n"
-"you can do so by fully-qualifying the
name with the --track option:\n"
+"If you meant to check out a remote
tracking branch on, e.g. 'origin',\n"
+"you can do so by fully qualifying the
name with the --track option:\n"
"\n"
- "git checkout --track
origin/"),
+ "git checkout --track
origin/\n"
Ævar Arnfjörð Bjarmason (8):
checkout tests: index should be clean after dwim checkout
checkout.h: wrap the arguments to unique_tracking_name()
checkout.c: introduce an *_INIT macro
checkout.c]: change "unique" member to "num_matches"
checkout: pass the "num_matches" up to callers
builtin/checkout.c: use "ret" variable for return
checkout: add advice for ambiguous "checkout "
checkout & worktree: introduce checkout.defaultRemote
Documentation/config.txt | 26 +++
Documentation/git-checkout.txt | 9 ++
Documentation/git-worktree.txt | 9 ++
advice.c | 2 ++
advice.h | 1 +
builtin/checkout.c | 41 ++-
builtin/worktree.c | 4 +--
checkout.c | 37 ++---
checkout.h | 4 ++-
t/t2024-checkout-dwim.sh | 59 ++
t/t2025-worktree-add.sh| 21
11 files changed, 197 insertions(+), 16 deletions(-)
--
2.17.0.290.gded63e768a
Re: [PATCH] t9104: kosherly remove remote refs
On Fri, Jun 1, 2018 at 7:08 AM, Christian Couder wrote: > As there are plans to implement other ref storage systems, > let's use a way to remove remote refs that does not depend > on refs being files. > > This makes it clear to readers that this test does not > depend on which ref backend is used. > > Suggested-by: Michael Haggerty > Helped-by: Jeff King > Signed-off-by: Christian Couder > --- > This was suggested and discussed in: > > https://public-inbox.org/git/20180525085906.ga2...@sigill.intra.peff.net/ > > t/t9104-git-svn-follow-parent.sh | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/t/t9104-git-svn-follow-parent.sh > b/t/t9104-git-svn-follow-parent.sh > index 9c49b6c1fe..5e0ad19177 100755 > --- a/t/t9104-git-svn-follow-parent.sh > +++ b/t/t9104-git-svn-follow-parent.sh > @@ -215,7 +215,9 @@ test_expect_success "multi-fetch continues to work" " > " > > test_expect_success "multi-fetch works off a 'clean' repository" ' > - rm -rf "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" && > + rm -rf "$GIT_DIR/svn" && > + git for-each-ref --format="option no-deref%0adelete %(refname)" > refs/remotes | > + git update-ref --stdin && > git reflog expire --all --expire=all && > mkdir "$GIT_DIR/svn" && > git svn multi-fetch > -- > 2.17.0.1035.g12039e008f +1 LGTM. Michael
Re: how exactly can git config section names contain periods?
On Sat, Jun 02, 2018 at 04:50:57AM -0400, Robert P. J. Day wrote: > On Fri, 1 Jun 2018, Jeff King wrote: > > > On Fri, Jun 01, 2018 at 04:14:12PM -0400, Robert P. J. Day wrote: > > > > > $ git config --global a.b.c.d.e rday > > > > > > huh ... seemed to work fine, and added this to my ~/.gitconfig: > > > > > > [a "b.c.d"] > > > e = rday > > > > > > as i see it, the first component is intgerpreted as the section > > > name, the last component is the variable/key(?) name, and > > > everything in between is treated as subsection(s), which is not at > > > all obvious from that Doc file, or from "man git-config". > > > > Yep, your understanding is correct. > > just to be precise regarding terminology, in my example above, is > "b.c.d" a single subsection, or does it refer to three subsections? > i'm guessing it refers to a single subsection, which is fine with me, > as long as it's very clearly explained that way in the docs. It's a single subsection. Each config key at most one subsection. -Peff
Lucrative Business Proposal
-- Dear Friend, I would like to discuss a very important issue with you. I am writing to find out if this is your valid email. Please, let me know if this email is valid Kind regards Adrien Saif Attorney to Quatif Group of Companies
Re: how exactly can git config section names contain periods?
On Fri, 1 Jun 2018, Jeff King wrote: > On Fri, Jun 01, 2018 at 04:14:12PM -0400, Robert P. J. Day wrote: > > > $ git config --global a.b.c.d.e rday > > > > huh ... seemed to work fine, and added this to my ~/.gitconfig: > > > > [a "b.c.d"] > > e = rday > > > > as i see it, the first component is intgerpreted as the section > > name, the last component is the variable/key(?) name, and > > everything in between is treated as subsection(s), which is not at > > all obvious from that Doc file, or from "man git-config". > > Yep, your understanding is correct. just to be precise regarding terminology, in my example above, is "b.c.d" a single subsection, or does it refer to three subsections? i'm guessing it refers to a single subsection, which is fine with me, as long as it's very clearly explained that way in the docs. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Lucrative Business Proposal
-- Dear Friend, I would like to discuss a very important issue with you. I am writing to find out if this is your valid email. Please, let me know if this email is valid Kind regards Adrien Saif Attorney to Quatif Group of Companies
Re: [PATCH] branch: issue "-l" deprecation warning after pager starts
On Sat, Jun 02, 2018 at 06:46:31AM +0200, Duy Nguyen wrote:
> > if (used_deprecated_reflog_option) {
> > - warning("the '-l' alias for '--create-reflog' is
> > deprecated;");
> > - warning("it will be removed in a future version of Git");
> > + if (list) {
> > + warning("the '-l' option is an alias for
> > '--create-reflog' and");
> > + warning("has no effect in list mode. This option
> > will soon be");
> > + warning("removed and you should omit it (or use
> > '--list' instead).");
> > + } else {
> > + warning("the '-l' alias for '--create-reflog' is
> > deprecated;");
> > + warning("it will be removed in a future version of
> > Git");
> > + }
>
> This is already in 'next', but could you do a follow up patch to mark
> these strings for translation? While at there, concatenating them into
> full sentences would also help translators.
Already being discussed elsewhere on the thread; we're hoping for a
warning() that will auto-prefix each one.
That said, I think I'm going to re-roll this in the direction discussed
elsewhere in the thread (skipping the warning for list-mode).
-Peff
Re: [PATCH v3] fast-export: fix regression skipping some merge-commits
On Fri, Jun 1, 2018 at 9:41 PM, Isaac Chou wrote: > Hello, I need help on this topic again. I need to inform our customers what > release this issue will be addressed in. I checked the 2.17.1 binary release > recently and found that the fix is not included. Can someone help me with > that information or point me to a document that I can use to determine it > myself? It's in 2.18.0-rc0, so it should be in the next 2.18.0 release (unless something horrible happens but very unlikely). -- Duy
Re: [PATCH 02/22] archive-zip.c: mark more strings for translation
On Sat, Jun 2, 2018 at 6:32 AM, Nguyễn Thái Ngọc Duy wrote:
> if (pathlen > 0x) {
> - return error("path too long (%d chars, SHA1: %s): %s",
> + return error(_("path too long (%d chars, SHA1: %s): %s"),
> (int)pathlen, oid_to_hex(oid), path);
Off topic. Brian, do we have a common term to use here (i.e. in user
facing messages) instead of "SHA1" after we move away from it? Is it
still "object name", or "hash" or some other fancy term?
--
Duy
Re: [PATCH] upload-pack: reject shallow requests that would return nothing
On Mon, May 28, 2018 at 8:48 PM, Duy Nguyen wrote: > On Mon, May 28, 2018 at 7:55 AM, Junio C Hamano wrote: >> Nguyễn Thái Ngọc Duy writes: >> >>> To avoid this, if rev-list returns nothing, we abort the clone/fetch. >>> The user could adjust their request (e.g. --shallow-since further back >>> in the past) and retry. >> >> Yeah, that makes sense. >> >>> Another possible option for this case is to fall back to a default >>> depth (like depth 1). But I don't like too much magic that way because >>> we may return something unexpected to the user. >> >> I agree that it would be a horrible fallback. I actually am >> wondering if we should just silently return no objects without even >> telling the user there is something unexpected happening. After >> all, the user may well be expecting with --shallow-since that is too >> recent that the fetch may not result in pulling anything new, and >> giving a "die" message, which now needs to be distinguished from >> other forms of die's like network connectivity or auth failures, is >> not all that helpful. > > An empty fetch is probably ok (though I would need to double check if > anything bad would happen or git-fetch would give some helpful > suggestion). git-clone on the other hand should actually clean this up > with a good advice. I'll need to check and come back with v2 later. It turns out harder than I thought. Cutting history and want/have negotiation are separate but must be consistent. If during the negotiation you said "I'm giving you this ref at this SHA-1" then you send nothing back, the client will complain at connectivity check. It expects all the objects that lead to said SHA-1. Part of the problem is we advertise refs very early, before accepting shallow requests and it's kinda hard to tell the user "ok with this set of shallow requests, only these refs are actually valid and could be sent back to you" before the want/have negotiation starts. At the current state, the only thing we could do is tell the user "nak you can't have that ref" even if we advertise the ref. This might confuse clients and does not sound great. I think for now die() may be a good enough quick fix. I'm not sure if it's worth messing with the want/have negotiation just to cover this rare edge case. -- Duy
