something about git clone
hello! i am a chinese. i have some question for you ,i want to know how 'git clone'work, it likes downloader? or other things. i think it'sdown speed is heigher,so i email to you . thanks ,and my english is so poor.:P
Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer wrote: > This round should fix all the UI issues Eric found in the last round. > The changes I made in a bit more detail: > > - added a new commit introducing a new hidden --show-new-head-line > flag in 'git reset'. This is used to suppress the "HEAD is now at > ..." line that 'git reset --hard' usually prints, so we can replace > it with our own "New worktree HEAD is now at ..." line instead, > while keeping the progress indicator for larger repositories. As with Junio, I'm fine with this hidden option (for now), however, I think you can take this a step further. Rather than having a (hidden) git-reset option which suppresses "HEAD is now at...", instead have a (hidden) option which augments the message. For example, --new-head-desc="New worktree" would make it output "New worktree HEAD is now at...". Changes to builtin/reset.c to support this would hardly be larger than the changes you already made. The major benefit is that patch 3/6 no longer has to duplicate the code from builtin/reset.c:print_new_head_line() just to print its own "New worktree HEAD is now at..." message. (As for the argument that "git worktree add" must duplicate that code because it wants the message on stderr, whereas git-reset prints it to stdout, I don't see why git-worktree puts those messages to stderr in the first place. As far as I can tell, it would be equally valid to print them to stdout.) > Some examples of the new UI behaviour here for reference: > > - guess-remote mode > > $ git worktree add --guess-remote ../next > Creating branch 'next' > Branch 'next' set up to track remote branch 'next' from 'origin'. > New worktree HEAD is now at caa68db14 Merge branch > 'sb/packfiles-in-repository' into next > > - original dwim (create a branch based on the current HEAD) > > $ git worktree add ../test > Creating branch 'test' > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone' > > - new dwim (check out existing branch) > > $ git worktree add ../test > Checking out branch 'test' > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone' > > - no new branch created > > $ git worktree add ../test2 origin/master > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone' I like the "creating" or "checking out" messages we now get for all the DWIM cases. I wonder if it would make sense to print "Checkout out blah..." for this case too. It's certainly not necessary since the user specified explicitly, but it would make the UI even more consistent, and address your subsequent comment about missing context above the "Checking out files: ...%" line for this case. Thoughts? > Compare this to the old UI (new dwim omitted, as there's no old > version of that): Thanks for contrasting the new with the old. The new output is nicer and more helpful. > The one thing we are loosing is a context line before "Checking out > files:", if no new branch is created. Personally I feel like that's > acceptable, as the user just used the 'git worktree add' command, so > it should be intuitive where those files are being checked out.
Re: [PATCH v6 3/6] worktree: improve message when creating a new worktree
On Sat, Mar 31, 2018 at 11:18 AM, Thomas Gummerer wrote: > [...] > Fix these inconsistencies, and no longer show the identifier by making > the 'git reset --hard' call not print the "HEAD is now at ..." line > using the newly introduced flag from the previous commit, and printing > the message directly from the builtin command instead. > > Signed-off-by: Thomas Gummerer > --- > diff --git a/builtin/worktree.c b/builtin/worktree.c > @@ -322,12 +320,22 @@ static int add_worktree(const char *path, const char > *refname, > argv_array_pushl(&cp.args, "reset", "--hard", NULL); > + argv_array_push(&cp.args, "--no-show-new-head-line"); > cp.env = child_env.argv; > ret = run_command(&cp); > if (ret) > goto done; > } > > + fprintf(stderr, _("New worktree HEAD is now at %s"), > + find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV)); > + > + strbuf_reset(&sb); > + pp_commit_easy(CMIT_FMT_ONELINE, commit, &sb); > + if (sb.len > 0) > + fprintf(stderr, " %s", sb.buf); > + fputc('\n', stderr); > + > is_junk = 0; > FREE_AND_NULL(junk_work_tree); > FREE_AND_NULL(junk_git_dir); Generally speaking, code such as this probably ought to be inserted outside of the is_junk={1,0} context in order to keep that critical section as small as possible. However, as mentioned in my response to the v6 cover letter[1], I think this chunk of new code can just go away entirely if git-reset is made to print the customized message on git-worktree's behalf. [1]: https://public-inbox.org/git/capig+cq8vzdycumo-qoexndbgqgegj2bpmpa-y0vhgct_br...@mail.gmail.com/
[PATCH v1 2/2] t/perf: add scripts to bisect performance regressions
The new bisect_regression script can be used to automatically bisect performance regressions. It will pass the new bisect_run_script to `git bisect run`. Signed-off-by: Christian Couder --- t/perf/bisect_regression | 73 t/perf/bisect_run_script | 47 ++ 2 files changed, 120 insertions(+) create mode 100755 t/perf/bisect_regression create mode 100755 t/perf/bisect_run_script diff --git a/t/perf/bisect_regression b/t/perf/bisect_regression new file mode 100755 index 00..a94d9955d0 --- /dev/null +++ b/t/perf/bisect_regression @@ -0,0 +1,73 @@ +#!/bin/sh + +# Read a line coming from `./aggregate.perl --sort-by regression ...` +# and automatically bisect to find the commit responsible for the +# performance regression. +# +# Lines from `./aggregate.perl --sort-by regression ...` look like: +# +# +100.0% p7821-grep-engines-fixed.1 0.04(0.10+0.03) 0.08(0.11+0.08) v2.14.3 v2.15.1 +# +33.3% p7820-grep-engines.1 0.03(0.08+0.02) 0.04(0.08+0.02) v2.14.3 v2.15.1 +# + +die () { + echo >&2 "error: $*" + exit 1 +} + +while [ $# -gt 0 ]; do + arg="$1" + case "$arg" in + --help) + echo "usage: $0 [--config file] [--subsection subsection]" + exit 0 + ;; + --config) + shift + GIT_PERF_CONFIG_FILE=$(cd "$(dirname "$1")"; pwd)/$(basename "$1") + export GIT_PERF_CONFIG_FILE + shift ;; + --subsection) + shift + GIT_PERF_SUBSECTION="$1" + export GIT_PERF_SUBSECTION + shift ;; + --*) + die "unrecognised option: '$arg'" ;; + *) + die "unknown argument '$arg'" + ;; + esac +done + +read -r regression subtest oldtime newtime oldrev newrev + +test_script=$(echo "$subtest" | sed -e 's/\(.*\)\.[0-9]*$/\1.sh/') +test_number=$(echo "$subtest" | sed -e 's/.*\.\([0-9]*\)$/\1/') + +# oldtime and newtime are decimal number, not integers + +oldtime=$(echo "$oldtime" | sed -e 's/^\([0-9]\+\.[0-9]\+\).*$/\1/') +newtime=$(echo "$newtime" | sed -e 's/^\([0-9]\+\.[0-9]\+\).*$/\1/') + +test $(echo "$newtime" "$oldtime" | awk '{ print ($1 > $2) }') = 1 || + die "New time '$newtime' shoud be greater than old time '$oldtime'" + +tmpdir=$(mktemp -d -t bisect_regression_XX) || die "Failed to create temp directory" +echo "$oldtime" >"$tmpdir/oldtime" || die "Failed to write to '$tmpdir/oldtime'" +echo "$newtime" >"$tmpdir/newtime" || die "Failed to write to '$tmpdir/newtime'" + +# Bisecting must be performed from the top level directory (even with --no-checkout) +( + toplevel_dir=$(git rev-parse --show-toplevel) || die "Failed to find top level directory" + cd "$toplevel_dir" || die "Failed to cd into top level directory '$toplevel_dir'" + + git bisect start --no-checkout "$newrev" "$oldrev" || die "Failed to start bisecting" + + git bisect run t/perf/bisect_run_script "$test_script" "$test_number" "$tmpdir" + res="$?" + + git bisect reset + + exit "$res" +) diff --git a/t/perf/bisect_run_script b/t/perf/bisect_run_script new file mode 100755 index 00..038255df4b --- /dev/null +++ b/t/perf/bisect_run_script @@ -0,0 +1,47 @@ +#!/bin/sh + +script="$1" +test_number="$2" +info_dir="$3" + +# This aborts the bisection immediately +die () { + echo >&2 "error: $*" + exit 255 +} + +bisect_head=$(git rev-parse --verify BISECT_HEAD) || die "Failed to find BISECT_HEAD ref" + +script_number=$(echo "$script" | sed -e "s/^p\([0-9]*\).*\$/\1/") || die "Failed to get script number for '$script'" + +oldtime=$(cat "$info_dir/oldtime") || die "Failed to access '$info_dir/oldtime'" +newtime=$(cat "$info_dir/newtime") || die "Failed to access '$info_dir/newtime'" + +cd t/perf || die "Failed to cd into 't/perf'" + +result_file="$info_dir/perf_${script_number}_${bisect_head}_results.txt" + +GIT_PERF_DIRS_OR_REVS="$bisect_head" +export GIT_PERF_DIRS_OR_REVS + +./run "$script" >"$result_file" 2>&1 || die "Failed to run perf test '$script'" + +rtime=$(sed -n "s/^$script_number\.$test_number:.*\([0-9]\+\.[0-9]\+\)(.*).*\$/\1/p" "$result_file") + +echo "newtime: $newtime" +echo "rtime: $rtime" +echo "oldtime: $oldtime" + +# Compare ($newtime - $rtime) with ($rtime - $oldtime) +# Times are decimal number, not integers + +if test $(echo "$newtime" "$rtime" "$oldtime" | awk '{ print ($1 - $2 > $2 - $3) }') = 1 +then + # Current commit is considered "good/old" + echo "$rtime" >"$info_dir/oldtime" + exit 0 +else + # Current commit is considered "bad/new" + echo "$rtime" >"$info_dir/newtime" + exit 1 +fi -- 2.17.0.rc1.36.g098d832c9.dirty
[PATCH v1 1/2] perf/run: add --subsection option
This new option makes it possible to run perf tests as defined in only one subsection of a config file. Signed-off-by: Christian Couder --- t/perf/run | 56 -- 1 file changed, 46 insertions(+), 10 deletions(-) diff --git a/t/perf/run b/t/perf/run index 213da5d6b9..9aaa733c77 100755 --- a/t/perf/run +++ b/t/perf/run @@ -1,21 +1,34 @@ #!/bin/sh -case "$1" in +die () { + echo >&2 "error: $*" + exit 1 +} + +while [ $# -gt 0 ]; do + arg="$1" + case "$arg" in + --) + break ;; --help) - echo "usage: $0 [--config file] [other_git_tree...] [--] [test_scripts]" - exit 0 - ;; + echo "usage: $0 [--config file] [--subsection subsec] [other_git_tree...] [--] [test_scripts]" + exit 0 ;; --config) shift GIT_PERF_CONFIG_FILE=$(cd "$(dirname "$1")"; pwd)/$(basename "$1") export GIT_PERF_CONFIG_FILE shift ;; -esac - -die () { - echo >&2 "error: $*" - exit 1 -} + --subsection) + shift + GIT_PERF_SUBSECTION="$1" + export GIT_PERF_SUBSECTION + shift ;; + --*) + die "unrecognised option: '$arg'" ;; + *) + break ;; + esac +done run_one_dir () { if test $# -eq 0; then @@ -172,9 +185,32 @@ get_subsections "perf" >test-results/run_subsections.names if test $(wc -l /dev/null || + die "subsection '$GIT_PERF_SUBSECTION' not found in '$GIT_PERF_CONFIG_FILE'" + + egrep "^$GIT_PERF_SUBSECTION\$" test-results/run_subsections.names | while read -r subsec + do + ( + GIT_PERF_SUBSECTION="$subsec" + export GIT_PERF_SUBSECTION + echo " Run for subsection '$GIT_PERF_SUBSECTION' " + run_subsection "$@" + ) + done else while read -r subsec do -- 2.17.0.rc1.36.g098d832c9.dirty
[PATCH v1 0/2] t/perf: bisect performance regressions
This is a small patch series on top of cc/perf-aggregate-sort, which is next, to add scripts that make it much easier to bisect performance regressions. For example if you ran perf test using the perf.conf config file that contains a "with libpcre", then using: $ ./aggregate.perl --subsection "with libpcre" --sort-by=regression v2.15.1 v2.16.2 v2.17.0 you can get a line in the output like: +8.2% p5302-pack-index.6 14.33(44.50+0.77) 15.50(43.83+0.94) v2.16.2 v2.17.0 and you can now use this output line to bisect where the regression comes from using: echo "+8.2% p5302-pack-index.6 14.33(44.50+0.77) 15.50(43.83+0.94) v2.16.2 v2.17.0" | ./bisect_regression --config perf.conf --subsection "with libpcre" Caveats: You must make sure that the times are consistent between runs and that there is a significant time difference between the "good" runs and the "bad" runs. For example the following is not easily bisectable: +100.0% p-perf-lib-sanity.2 0.01(0.01+0.01) 0.02(0.02+0.00) v2.15.1 v2.16.2 as the rtime went from 0.01 s to 0.02 s and we only have 0.01 s precision. Christian Couder (2): perf/run: add --subsection option t/perf: add scripts to bisect performance regressions t/perf/bisect_regression | 73 t/perf/bisect_run_script | 47 ++ t/perf/run | 56 -- 3 files changed, 166 insertions(+), 10 deletions(-) create mode 100755 t/perf/bisect_regression create mode 100755 t/perf/bisect_run_script -- 2.17.0.rc1.36.g098d832c9.dirty
Re: [PATCH v1 1/2] perf/run: add --subsection option
On Sun, Apr 8, 2018 at 5:35 AM, Christian Couder wrote: > This new option makes it possible to run perf tests as defined > in only one subsection of a config file. > > Signed-off-by: Christian Couder > --- > diff --git a/t/perf/run b/t/perf/run > @@ -1,21 +1,34 @@ > +while [ $# -gt 0 ]; do I was going to ask if you meant to use 'test' here rather than '[', however, I see that this script already has a mix of the two, so... Likewise, scripts in Git usually omit the semicolon and place 'do' on its own line, however, again I see that this script has a mix of the two styles, so...
Re: [PATCH v6 6/6] worktree: teach "add" to check out existing branches
On Sat, Mar 31, 2018 at 11:18 AM, Thomas Gummerer wrote: > [...] > However we can do a little better than that, and check the branch out if > it is not checked out anywhere else. This will help users who just want > to check an existing branch out into a new worktree, and save a few > keystrokes. > [...] > Signed-off-by: Thomas Gummerer > --- > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > @@ -61,8 +61,13 @@ $ git worktree add --track -b > / > If `` is omitted and neither `-b` nor `-B` nor `--detach` used, > +then, as a convenience, a worktree with a branch named after > +`$(basename )` (call it ``) is created. I had a hard time digesting this. I _think_ it wants to say: If `` is omitted and neither `-b` nor `-B` nor `--detach` is used, then, as a convenience, the new worktree is associated with a branch (call it ``) named after `$(basename )`. > If `` > +doesn't exist, a new branch based on HEAD is automatically created as > +if `-b ` was given. If `` exists in the repository, Maybe: s/exists in the repository/does exist/ Or: s/.../is a local branch/ Though, the latter may be getting too pedantic. > +it will be checked out in the new worktree, if it's not checked out > +anywhere else, otherwise the command will refuse to create the > +worktree (unless `--force` is used). > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh > @@ -198,13 +198,24 @@ test_expect_success '"add" with omitted' ' > +test_expect_success '"add" checks out existing branch of dwimd name' ' > + git branch dwim HEAD~1 && > + git worktree add dwim && > + test_cmp_rev HEAD~1 dwim && > + ( > + cd dwim && > + test_cmp_rev dwim HEAD Nit: Argument order of the two test_cmp_rev() invocations differs. > + ) > +' > + > +test_expect_success '"add " dwim fails with checked out branch' ' > + git checkout -b test-branch && > + test_must_fail git worktree add test-branch && > + test_path_is_missing test-branch > +' > + > +test_expect_success '"add --force" with existing dwimd name doesnt die' ' > + git worktree add --force test-branch > ' Maybe make this last test slightly more robust by having it "git checkout test-branch" before "git worktree add ..." to protect against someone inserting a new test (which checks out some other branch) between these two. Probably not worth a re-roll.
getting pull and push URLs for the current branch
Hello, is there a way to get remote url for the current branch? That is the url that will be used when I call `git pull`. It doesn't seem to be a particularly easy task. Thank you! clime
Re: [PATCH v7 04/14] graph: add commit graph design document
Derrick Stolee writes: > From: Derrick Stolee > > Add Documentation/technical/commit-graph.txt with details of the planned > commit graph feature, including future plans. That's in my opinion a very good idea. It would help anyone trying to add to and extend this feature. > Signed-off-by: Derrick Stolee > --- > Documentation/technical/commit-graph.txt | 163 +++ > 1 file changed, 163 insertions(+) > create mode 100644 Documentation/technical/commit-graph.txt > > diff --git a/Documentation/technical/commit-graph.txt > b/Documentation/technical/commit-graph.txt > new file mode 100644 > index 00..0550c6d0dc > --- /dev/null > +++ b/Documentation/technical/commit-graph.txt > @@ -0,0 +1,163 @@ > +Git Commit Graph Design Notes > += > + > +Git walks the commit graph for many reasons, including: > + > +1. Listing and filtering commit history. > +2. Computing merge bases. > + > +These operations can become slow as the commit count grows. The merge > +base calculation shows up in many user-facing commands, such as 'merge-base' > +or 'status' and can take minutes to compute depending on history shape. > + > +There are two main costs here: > + > +1. Decompressing and parsing commits. > +2. Walking the entire graph to satisfy topological order constraints. > + > +The commit graph file is a supplemental data structure that accelerates > +commit graph walks. If a user downgrades or disables the 'core.commitGraph' > +config setting, then the existing ODB is sufficient. This is a good explanation of why we want to have this, and why in this form. I really like the "Related links" with summary. > The file is stored > +as "commit-graph" either in the .git/objects/info directory or in the info > +directory of an alternate. That is a good thing. Do I understand it correctly that Git would use first "commit-graph" file that it would encounter, or would it merge information from all alternates (including perhaps self)? What if repository is a subtree-merge of different repositories, with each listed separately as alternate? > + > +The commit graph file stores the commit graph structure along with some > +extra metadata to speed up graph walks. By listing commit OIDs in lexi- > +cographic order, we can identify an integer position for each commit and > +refer to the parents of a commit using those integer positions. We use > +binary search to find initial commits and then use the integer positions > +for fast lookups during the walk. It might be worth emphasizing here that fast access using integer positions for commits in the chunk is possible only if chunk used fixed-width format (each commit taking the same amount of space -- which for example is not true for packfile). > + > +A consumer may load the following info for a commit from the graph: > + > +1. The commit OID. > +2. The list of parents, along with their integer position. > +3. The commit date. > +4. The root tree OID. > +5. The generation number (see definition below). > + > +Values 1-4 satisfy the requirements of parse_commit_gently(). Good to have it here. It is nice to know why 1-4 are needed to be in the "commit-graph" structure. Do I understand it correctly that this feature is what makes porting Git to start using "commit-graph" information easy, because it is single point of entry, isn't it? > + > +Define the "generation number" of a commit recursively as follows: > + > + * A commit with no parents (a root commit) has generation number one. > + > + * A commit with at least one parent has generation number one more than > + the largest generation number among its parents. > + > +Equivalently, the generation number of a commit A is one more than the > +length of a longest path from A to a root commit. The recursive definition > +is easier to use for computation and observing the following property: > + > +If A and B are commits with generation numbers N and M, respectively, > +and N <= M, then A cannot reach B. That is, we know without searching > +that B is not an ancestor of A because it is further from a root commit > +than A. Because generation numbers from the "commit-graph" may not cover all commits (recent commits can have no generation number information), I think it would be good idea to state what happens then. Because of how generation numbers are defined, if commit A has generation number provided, then all ancestors also have generation number information. Thus: If A is a commit with generation number N, and B is a commit without generation number information, then A cannot reach B. That is, we know without searching that B is not an ancestor of A because if it was, it would have generation number information. Additionally (but that might be not worth adding here): If A is a commit without generation number information, and B is a commit with generation nu
Re: getting pull and push URLs for the current branch
On Sun, Apr 08 2018, Michal Novotny wrote: > is there a way to get remote url for the current branch? That is the > url that will be used when I call `git pull`. It doesn't seem to be a > particularly easy task. You'd do something like this (sans error checking): git remote get-url $(git config branch.$(git rev-parse --abbrev-ref HEAD).remote) I.e. your HEAD may have remote tracking info set up, this'll get the remote URL for the remote it points at.
Re: something about git clone
On Sun, Apr 08 2018, jiangwei zhao wrote: > hello! > i am a chinese. > i have some question for you ,i want to know how 'git clone'work, > it likes downloader? or other things. i think it'sdown speed is > heigher,so i email to you . > thanks ,and my english is so poor.:P You will not get the same speed from "git clone" as "wget" of a static file, since the server may need to do a lot more work to serve you, so it will be slower than downloading a static file.
Re: [PATCH v7 07/14] commit-graph: implement git-commit-graph write
Derrick Stolee writes: > +# Current graph structure: > +# > +# __M3___ > +# / | \ > +# 3 M1 5 M2 7 > +# |/ \|/ \| > +# 246 > +# |___// > +# 1 Good, so we are testing EDGE chunk, because the commit graph has octopus merge in it (with more than two parents). Do we need to test multiple roots, and/or independent (orphan) branches cases? Best, -- Jakub Narębski
[PATCH v12 3/4] ref-filter: factor ref_array pushing into its own function
From: Jeff King In preparation for callers constructing their own ref_array structs, let's move our own internal push operation into its own function. While we're at it, we can replace REALLOC_ARRAY() with ALLOC_GROW(), which should give the growth operation amortized linear complexity (as opposed to growing by one, which is potentially quadratic, though in-place realloc growth often makes this faster in practice). Signed-off-by: Jeff King Signed-off-by: Harald Nordgren --- ref-filter.c | 16 +--- ref-filter.h | 8 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index c1c3cc948..6e9328b27 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1840,6 +1840,18 @@ static struct ref_array_item *new_ref_array_item(const char *refname, return ref; } +struct ref_array_item *ref_array_push(struct ref_array *array, + const char *refname, + const struct object_id *oid) +{ + struct ref_array_item *ref = new_ref_array_item(refname, oid); + + ALLOC_GROW(array->items, array->nr + 1, array->alloc); + array->items[array->nr++] = ref; + + return ref; +} + static int ref_kind_from_refname(const char *refname) { unsigned int i; @@ -1930,13 +1942,11 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid, * to do its job and the resulting list may yet to be pruned * by maxcount logic. */ - ref = new_ref_array_item(refname, oid); + ref = ref_array_push(ref_cbdata->array, refname, oid); ref->commit = commit; ref->flag = flag; ref->kind = kind; - REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1); - ref_cbdata->array->items[ref_cbdata->array->nr++] = ref; return 0; } diff --git a/ref-filter.h b/ref-filter.h index 68268f9eb..76cf87cb6 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -135,4 +135,12 @@ void setup_ref_filter_porcelain_msg(void); void pretty_print_ref(const char *name, const struct object_id *oid, const struct ref_format *format); +/* + * Push a single ref onto the array; this can be used to construct your own + * ref_array without using filter_refs(). + */ +struct ref_array_item *ref_array_push(struct ref_array *array, + const char *refname, + const struct object_id *oid); + #endif /* REF_FILTER_H */ -- 2.14.3 (Apple Git-98)
Re: [PATCH v11 1/4] ref-filter: use "struct object_id" consistently
Thanks for your very thorough review Eric! I think I address all the points, but if I missed something please let me know. On Sun, Apr 8, 2018 at 3:06 AM, Eric Sunshine wrote: > > You incorrectly dropped Peff's sign-off[1] when re-sending the patches > he authored in the series. And, your sign-off should follow his. I just rebased on https://github.com/peff/git/tree/jk/ref-array-push where there were no sign-off tags. But I amended the commit messages now to add the sign-off. And also added my own sign-off. > Also, if you made any changes to Peff's patch, it's a good idea to > state so with a bracketed comment at the end of the commit message > (before the sign-offs). For instance: > > [hn: tweaked doodle blap] > > or such. > > [1]: > https://public-inbox.org/git/20180406185831.ga11...@sigill.intra.peff.net/ As a sign of respect I probably wouldn't want to tweak the patches. They currently work well together with my implementation, so there is no need. > This "last becomes primary key" feels counterintuitive to me, however, > I see it mirrors precedence of other Git commands. > > In what situations would it make sense to specify --sort= multiple > times in the context of ls-remote? If there are none, then I wonder if > this should instead be documented as "last wins"... > > To what does "Also" refer? This was copied wholesale from Documentation/git-tag.txt. I minimized the text now and removed all the references to using 'sort' multiple times. Although you technically could use multiple keys, since 'ls-remote' only outputs two columns, I guess it's never needed. > What does "not work" mean in this context? Will the command crash > outright? Will it emit a suitable error message or warning? Will the > sorting be somehow dysfunctional? This will be the output when trying to sort by commiterdate: From g...@github.com:git/git.git fatal: missing object f0d0fd3a5985d5e588da1e1d11c85fba0ae132f8 for refs/pull/10/head I added a a note in the documentation saying that it will "cause a failure". Should we be even more explicit than that? This is one side-effect of borrowing the ref-filter logic in this patch. We inherit things that won't work on remotes. > It seems like the sentence "The keys supported..." should go above the > "Also supports..." sentence for this explanation to be more cohesive. > > Finally, how about adding a linkgit:git-for-each-ref[1] to give the > user an easy way to access the referenced documentation. For instance: > > The keys supported are the same as those accepted by the > `--sort=` option of linkgit:git-for-each-ref[1], except... > Added a "linkgit" to git-for-each-ref. And a "See also" section at the bottom. > Can we have a more meaningful name than 'array'? Even a name a simple > as 'refs' would convey more information. I think 'refs' would be confusing considering that the return value of 'transport_get_remote_refs' is stored as 'ref'. I renamed it to 'ref_array'. I hope it's not a problem that is shadows its struct name. But I've seen us this naming paradigm in other places -- and in this file. > Do we need to worry about freeing memory allocated by these two lines? > > More generally, do we care that 'array' and 'sorting' are being > leaked? If not, perhaps they should be annotated by UNLEAK. Since 'cmd_ls_remote' is always (?) called from another process I don't think we need to worry explicitly freeing the memory. I added UNLEAK annotations to my code. > This test script is already filled with these hardcoded SHA-1's, so > this is not a new problem, but work is under way to make it possible > to replace SHA-1 with some other hashing algorithm. Test scripts are > being retrofitted to avoid hard-coding them; instead determining the > hash value dynamically (for example, [1]). It would be nice if the new > tests followed suit, thus saving someone else extra work down the > road. (This, itself, is not worth a re-roll, but something to consider > if you do re-roll.) > > [1]: > https://public-inbox.org/git/20180325192055.841459-10-sand...@crustytoothpaste.net/ Added 'git rev-parse' calls to my tests do decrease the reliance on SHA-1's. For the test ls-remote --symref I couldn't figure out a way to extract the hash for 'refs/remotes/origin/HEAD' so I re-used HEAD. I tried different ways of fetching origin, making another commit and pushing, but origin/HEAD is still unavailable. By calling 'git fetch origin' before the test, at least I am able to extract 'git rev-parse refs/remotes/origin/master'. This all makes the tests a bit more complicated, so I hope it helps us in the future! :) > Do we want a test verifying that multiple sort keys are respected? (Is > it even possible to construct such a test?) To add to my previous point: Since 'ls-remote' outputs only two columns, I don't see a use case for multiple keys. And I don't know what the test would look like either. >> @@ -131,7 +167,7 @@ test_expect_success 'Report no-match with
[PATCH v12 1/4] ref-filter: use "struct object_id" consistently
From: Jeff King Internally we store a "struct object_id", and all of our callers have one to pass us. But we insist that they peel it to its bare-sha1 hash, which we then hashcpy() into place. Let's pass it around as an object_id, which future-proofs us for a post-sha1 world. Signed-off-by: Jeff King Signed-off-by: Harald Nordgren --- builtin/tag.c| 2 +- builtin/verify-tag.c | 2 +- ref-filter.c | 10 +- ref-filter.h | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index da186691e..42278f516 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -117,7 +117,7 @@ static int verify_tag(const char *name, const char *ref, return -1; if (format->format) - pretty_print_ref(name, oid->hash, format); + pretty_print_ref(name, oid, format); return 0; } diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index ad7b79fa5..6fa04b751 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -72,7 +72,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) } if (format.format) - pretty_print_ref(name, oid.hash, &format); + pretty_print_ref(name, &oid, &format); } return had_error; } diff --git a/ref-filter.c b/ref-filter.c index 45fc56216..ade97a848 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1826,12 +1826,12 @@ static const struct object_id *match_points_at(struct oid_array *points_at, /* Allocate space for a new ref_array_item and copy the objectname and flag to it */ static struct ref_array_item *new_ref_array_item(const char *refname, -const unsigned char *objectname, +const struct object_id *oid, int flag) { struct ref_array_item *ref; FLEX_ALLOC_STR(ref, refname, refname); - hashcpy(ref->objectname.hash, objectname); + oidcpy(&ref->objectname, oid); ref->flag = flag; return ref; @@ -1927,7 +1927,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid, * to do its job and the resulting list may yet to be pruned * by maxcount logic. */ - ref = new_ref_array_item(refname, oid->hash, flag); + ref = new_ref_array_item(refname, oid, flag); ref->commit = commit; REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1); @@ -2165,11 +2165,11 @@ void show_ref_array_item(struct ref_array_item *info, putchar('\n'); } -void pretty_print_ref(const char *name, const unsigned char *sha1, +void pretty_print_ref(const char *name, const struct object_id *oid, const struct ref_format *format) { struct ref_array_item *ref_item; - ref_item = new_ref_array_item(name, sha1, 0); + ref_item = new_ref_array_item(name, oid, 0); ref_item->kind = ref_kind_from_refname(name); show_ref_array_item(ref_item, format); free_array_item(ref_item); diff --git a/ref-filter.h b/ref-filter.h index 0d98342b3..68268f9eb 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -132,7 +132,7 @@ void setup_ref_filter_porcelain_msg(void); * Print a single ref, outside of any ref-filter. Note that the * name must be a fully qualified refname. */ -void pretty_print_ref(const char *name, const unsigned char *sha1, +void pretty_print_ref(const char *name, const struct object_id *oid, const struct ref_format *format); #endif /* REF_FILTER_H */ -- 2.14.3 (Apple Git-98)
[PATCH v12 4/4] ls-remote: create '--sort' option
Create a '--sort' option for ls-remote, based on the one from for-each-ref. This e.g. allows ref names to be sorted by version semantics, so that v1.2 is sorted before v1.10. Signed-off-by: Harald Nordgren --- Notes: Changes according to Eric Sunshine's code review Documentation/git-ls-remote.txt | 16 - builtin/ls-remote.c | 30 +--- t/t5512-ls-remote.sh| 52 - 3 files changed, 88 insertions(+), 10 deletions(-) diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt index 5f2628c8f..80a09b518 100644 --- a/Documentation/git-ls-remote.txt +++ b/Documentation/git-ls-remote.txt @@ -10,7 +10,7 @@ SYNOPSIS [verse] 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=] - [-q | --quiet] [--exit-code] [--get-url] + [-q | --quiet] [--exit-code] [--get-url] [--sort=] [--symref] [ [...]] DESCRIPTION @@ -60,6 +60,16 @@ OPTIONS upload-pack only shows the symref HEAD, so it will be the only one shown by ls-remote. +--sort=:: + Sort based on the key given. Prefix `-` to sort in descending order + of the value. Supports "version:refname" or "v:refname" (tag names + are treated as versions). The "version:refname" sort order can also + be affected by the "versionsort.suffix" configuration variable. + See linkgit:git-for-each-ref[1] for more sort options, but be aware + that because `ls-remote` deals only with remotes, any key like + `committerdate` that requires access to the object itself will + cause a failure. + :: The "remote" repository to query. This parameter can be either a URL or the name of a remote (see the GIT URLS and @@ -90,6 +100,10 @@ EXAMPLES c5db5456ae3b0873fc659c19fafdde22313cc441refs/tags/v0.99.2 7ceca275d047c90c0c7d5afb13ab97efdf51bd6erefs/tags/v0.99.3 +SEE ALSO + +linkgit:git-check-ref-format[1]. + GIT --- Part of the linkgit:git[1] suite diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c index 540d56429..d3851074c 100644 --- a/builtin/ls-remote.c +++ b/builtin/ls-remote.c @@ -1,6 +1,7 @@ #include "builtin.h" #include "cache.h" #include "transport.h" +#include "ref-filter.h" #include "remote.h" static const char * const ls_remote_usage[] = { @@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) int show_symref_target = 0; const char *uploadpack = NULL; const char **pattern = NULL; + int i; struct remote *remote; struct transport *transport; const struct ref *ref; + struct ref_array ref_array; + static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting; struct option options[] = { OPT__QUIET(&quiet, N_("do not print remote URL")), @@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL), OPT_BOOL(0, "get-url", &get_url, N_("take url..insteadOf into account")), + OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"), +N_("field name to sort on"), &parse_opt_ref_sorting), OPT_SET_INT_F(0, "exit-code", &status, N_("exit with exit code 2 if no matching refs are found"), 2, PARSE_OPT_NOCOMPLETE), @@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) OPT_END() }; + memset(&ref_array, 0, sizeof(ref_array)); + argc = parse_options(argc, argv, prefix, options, ls_remote_usage, PARSE_OPT_STOP_AT_NON_OPTION); dest = argv[0]; @@ -90,6 +98,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) if (get_url) { printf("%s\n", *remote->url); + UNLEAK(sorting); return 0; } @@ -98,20 +107,35 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack); ref = transport_get_remote_refs(transport); - if (transport_disconnect(transport)) + if (transport_disconnect(transport)) { + UNLEAK(sorting); return 1; + } if (!dest && !quiet) fprintf(stderr, "From %s\n", *remote->url); for ( ; ref; ref = ref->next) { + struct ref_array_item *item; if (!check_ref_type(ref, flags)) continue; if (!tail_match(pattern, ref->name)) continue; + item = ref_array_push(&ref_array, ref->name, &ref->old_oid); + item->symref = xstrdup_
[PATCH v12 2/4] ref-filter: make ref_array_item allocation more consistent
From: Jeff King We have a helper function to allocate ref_array_item structs, but it only takes a subset of the possible fields in the struct as initializers. We could have it accept an argument for _every_ field, but that becomes a pain for the fields which some callers don't want to set initially. Instead, let's be explicit that it takes only the minimum required to create the ref, and that callers should then fill in the rest themselves. Signed-off-by: Jeff King Signed-off-by: Harald Nordgren --- ref-filter.c | 18 +++--- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index ade97a848..c1c3cc948 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1824,15 +1824,18 @@ static const struct object_id *match_points_at(struct oid_array *points_at, return NULL; } -/* Allocate space for a new ref_array_item and copy the objectname and flag to it */ +/* + * Allocate space for a new ref_array_item and copy the name and oid to it. + * + * Callers can then fill in other struct members at their leisure. + */ static struct ref_array_item *new_ref_array_item(const char *refname, -const struct object_id *oid, -int flag) +const struct object_id *oid) { struct ref_array_item *ref; + FLEX_ALLOC_STR(ref, refname, refname); oidcpy(&ref->objectname, oid); - ref->flag = flag; return ref; } @@ -1927,12 +1930,13 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid, * to do its job and the resulting list may yet to be pruned * by maxcount logic. */ - ref = new_ref_array_item(refname, oid, flag); + ref = new_ref_array_item(refname, oid); ref->commit = commit; + ref->flag = flag; + ref->kind = kind; REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1); ref_cbdata->array->items[ref_cbdata->array->nr++] = ref; - ref->kind = kind; return 0; } @@ -2169,7 +2173,7 @@ void pretty_print_ref(const char *name, const struct object_id *oid, const struct ref_format *format) { struct ref_array_item *ref_item; - ref_item = new_ref_array_item(name, oid, 0); + ref_item = new_ref_array_item(name, oid); ref_item->kind = ref_kind_from_refname(name); show_ref_array_item(ref_item, format); free_array_item(ref_item); -- 2.14.3 (Apple Git-98)
Re: [PATCH v7 08/14] commit-graph: implement git commit-graph read
Derrick Stolee writes: [...] > EXAMPLES > > @@ -45,6 +51,12 @@ EXAMPLES > $ git commit-graph write > > > +* Read basic information from the commit-graph file. > ++ > + > +$ git commit-graph read > + It would be better to have example output of this command here, perhaps together with ASCII-art diagram of the code graph. [...] > + if (!graph) > + die("graph file %s does not exist", graph_name); > + FREE_AND_NULL(graph_name); Shouldn't the error message be marked up for translation, too? Regards, -- Jakub Narębski
Re: [PATCH v7 09/14] commit-graph: add core.commitGraph setting
Derrick Stolee writes: > From: Derrick Stolee > > The commit graph feature is controlled by the new core.commitGraph config > setting. This defaults to 0, so the feature is opt-in. Nice. That's how bitmaps feature was introduced, I think. I guess that in the future reading would be opt-out, isn't it, same as currently for bitmaps (pack.useBitmaps setting). > > The intention of core.commitGraph is that a user can always stop checking > for or parsing commit graph files if core.commitGraph=0. Shouldn't it be "false", not "0"? > Signed-off-by: Derrick Stolee > --- > Documentation/config.txt | 4 > cache.h | 1 + > config.c | 5 + > environment.c| 1 + > 4 files changed, 11 insertions(+) > > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 4e0cff87f6..e5c7013fb0 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -898,6 +898,10 @@ core.notesRef:: > This setting defaults to "refs/notes/commits", and it can be overridden by > the `GIT_NOTES_REF` environment variable. See linkgit:git-notes[1]. > > +core.commitGraph:: > + Enable git commit graph feature. Allows reading from the > + commit-graph file. > + Good. It would be nice to have "Defaults to false." and possibly also reference to "git-commit-graph(1)" manpage for more details, though. > core.sparseCheckout:: > Enable "sparse checkout" feature. See section "Sparse checkout" in > linkgit:git-read-tree[1] for more information. [...] -- Jakub Narębski
Re: [PATCH v7 13/14] commit-graph: build graph from starting commits
Derrick Stolee writes: > @@ -96,10 +101,12 @@ 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")); Here error message is marked for translation, which is not the case for other patches in the series. -- Jakub Narębski
Re: Is offloading to GPU a worthwhile feature?
Hello, Konstantin Ryabitsev writes: > This is an entirely idle pondering kind of question, but I wanted to > ask. I recently discovered that some edge providers are starting to > offer systems with GPU cards in them -- primarily for clients that need > to provide streaming video content, I guess. As someone who needs to run > a distributed network of edge nodes for a fairly popular git server, I > wondered if git could at all benefit from utilizing a GPU card for > something like delta calculations or compression offload, or if benefits > would be negligible. > > I realize this would be silly amounts of work. But, if it's worth it, > perhaps we can benefit from all the GPU computation libs written for > cryptocoin mining and use them for something good. :) The problem is that you need to transfer the data from the main memory (host memory) geared towards low-latency thanks to cache hierarchy, to the GPU memory (device memory) geared towards bandwidth and parallel access, and back again. So to make sense the time for copying data plus the time to perform calculations on GPU (and not all kinds of computations can be speed up on GPU -- you need fine-grained massively data-parallel task) must be less than time to perform calculations on CPU (with multi-threading). Also you would need to keep non-GPU and GPGPU code in sync. Some parts of code do not change much; and there also solutions to generate dual code from one source. Still, it might be good idea, -- Jakub Narębski
Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
On 04/08, Eric Sunshine wrote: > On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer > wrote: > > This round should fix all the UI issues Eric found in the last round. > > The changes I made in a bit more detail: > > > > - added a new commit introducing a new hidden --show-new-head-line > > flag in 'git reset'. This is used to suppress the "HEAD is now at > > ..." line that 'git reset --hard' usually prints, so we can replace > > it with our own "New worktree HEAD is now at ..." line instead, > > while keeping the progress indicator for larger repositories. > > As with Junio, I'm fine with this hidden option (for now), however, I > think you can take this a step further. Rather than having a (hidden) > git-reset option which suppresses "HEAD is now at...", instead have a > (hidden) option which augments the message. For example, > --new-head-desc="New worktree" would make it output "New worktree HEAD > is now at...". Changes to builtin/reset.c to support this would hardly > be larger than the changes you already made. > > The major benefit is that patch 3/6 no longer has to duplicate the > code from builtin/reset.c:print_new_head_line() just to print its own > "New worktree HEAD is now at..." message. (As for the argument that > "git worktree add" must duplicate that code because it wants the > message on stderr, whereas git-reset prints it to stdout, I don't see > why git-worktree puts those messages to stderr in the first place. As > far as I can tell, it would be equally valid to print them to stdout.) I didn't think of that, but I think that's nicer indeed. Will change. This will also be nicer when we're in a position to remove the hidden option and do all of this with internal functions. Then we can just re-use the new function that also takes a prefix at that point (after moving the function to 'libgit.a' of course. > > Some examples of the new UI behaviour here for reference: > > > > - guess-remote mode > > > > $ git worktree add --guess-remote ../next > > Creating branch 'next' > > Branch 'next' set up to track remote branch 'next' from 'origin'. > > New worktree HEAD is now at caa68db14 Merge branch > > 'sb/packfiles-in-repository' into next > > > > - original dwim (create a branch based on the current HEAD) > > > > $ git worktree add ../test > > Creating branch 'test' > > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone' > > > > - new dwim (check out existing branch) > > > > $ git worktree add ../test > > Checking out branch 'test' > > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone' > > > > - no new branch created > > > > $ git worktree add ../test2 origin/master > > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone' > > I like the "creating" or "checking out" messages we now get for all > the DWIM cases. I wonder if it would make sense to print "Checkout out > blah..." for this case too. It's certainly not necessary since the > user specified explicitly, but it would make the UI even > more consistent, and address your subsequent comment about missing > context above the "Checking out files: ...%" line for this case. > Thoughts? Let me think through some of the cases here, of 'git worktre add ' with various flags and what the UI would be with that added: - no flags: $ git worktree add ../test origin/master Checking out 'origin/master' Checking out files: ...% New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone' - -b branch: $ git worktree add -b test ../test origin/master Creating branch 'test' Checking out 'origin/master' Checking out files: ...% New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone' Would we want to omit the "Checking out ..." here? I'm leaning towards yes, but dunno? - --no-checkout $ git worktree add --no-checkout test ../test origin/master New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone' - Original dwim with --detach flag $ git worktree add --detach ../test Checking out 'c2a499e6c' Checking out files: ...% New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone' Looking at this, I'm not sure what's best here. I'm not sure I'm a fan of the duplicate "Checking out " message (I assume that's what you meant above, or did you mean just "Checkout ..."?) I als don't think it gives too much context compared to just "Checking out files: ...%". I think it gives a bit more context when that message is not displayed at all, as it shows whether files are checked out or not, but if we do that, when we create a new branch, the amount of output we'd display is getting a bit long, to the point where I suspect users would just not read it anymore. So I personally don't feel like this is worth it, even though it may give some context in some cases. But I'm also far from an expert in UI design, so if you (o
Re: [PATCH v6 6/6] worktree: teach "add" to check out existing branches
On 04/08, Eric Sunshine wrote: > On Sat, Mar 31, 2018 at 11:18 AM, Thomas Gummerer > wrote: > > [...] > > However we can do a little better than that, and check the branch out if > > it is not checked out anywhere else. This will help users who just want > > to check an existing branch out into a new worktree, and save a few > > keystrokes. > > [...] > > Signed-off-by: Thomas Gummerer > > --- > > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > > @@ -61,8 +61,13 @@ $ git worktree add --track -b > > / > > If `` is omitted and neither `-b` nor `-B` nor `--detach` used, > > +then, as a convenience, a worktree with a branch named after > > +`$(basename )` (call it ``) is created. > > I had a hard time digesting this. I _think_ it wants to say: > > If `` is omitted and neither `-b` nor `-B` nor > `--detach` is used, then, as a convenience, the new worktree is > associated with a branch (call it ``) named after > `$(basename )`. Yeah, this is what it wants to say, and what you have sounds much nicer, will change. > > If `` > > +doesn't exist, a new branch based on HEAD is automatically created as > > +if `-b ` was given. If `` exists in the repository, > > Maybe: s/exists in the repository/does exist/ > Or: s/.../is a local branch/ > > Though, the latter may be getting too pedantic. > > > +it will be checked out in the new worktree, if it's not checked out > > +anywhere else, otherwise the command will refuse to create the > > +worktree (unless `--force` is used). > > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh > > @@ -198,13 +198,24 @@ test_expect_success '"add" with omitted' ' > > +test_expect_success '"add" checks out existing branch of dwimd name' ' > > + git branch dwim HEAD~1 && > > + git worktree add dwim && > > + test_cmp_rev HEAD~1 dwim && > > + ( > > + cd dwim && > > + test_cmp_rev dwim HEAD > > Nit: Argument order of the two test_cmp_rev() invocations differs. > > > + ) > > +' > > + > > +test_expect_success '"add " dwim fails with checked out branch' ' > > + git checkout -b test-branch && > > + test_must_fail git worktree add test-branch && > > + test_path_is_missing test-branch > > +' > > + > > +test_expect_success '"add --force" with existing dwimd name doesnt die' ' > > + git worktree add --force test-branch > > ' > > Maybe make this last test slightly more robust by having it "git > checkout test-branch" before "git worktree add ..." to protect against > someone inserting a new test (which checks out some other branch) > between these two. Probably not worth a re-roll. As I'll have to re-roll anyway for the other suggestions, I'll change this as well. Thanks for your review!
[RFC PATCH 1/1] completion: load completion file for external subcommand
Adding external subcommands to Git is as easy as to put an executable file git-foo into PATH. Packaging such subcommands for a Linux distribution can be achieved by unpacking the executable into /usr/bin of the user's system. Adding system-wide completion scripts for new subcommands, however, can be a bit tricky. Since bash-completion started to use dynamical loading of completion scripts somewhere around v2.0, it is no longer sufficient to drop a completion script of a subcommand into the standard completions path, /usr/share/bash-completion/completions, since this script will not be loaded if called as a git subcommand. For example, look at https://bugs.gentoo.org/544722. To give a short summary: The popular git-flow subcommand provides a completion script, which gets installed as /usr/share/bash-completion/completions/git-flow. If you now type into a Bash shell: git flow You will not get any completions, because bash-completion only loads completions for git and git has no idea that git-flow is defined in another file. You have to load this script manually or trigger the dynamic loader with: git-flow # Please notice the dash instead of whitespace This will not complete anything either, because it only defines a Bash function, without generating completions. But now the correct completion script has been loaded and the first command can use the completions. So, the goal is now to teach the git completion script to consider the possibility of external completion scripts for subcommands, but of course without breaking current workflows. I think the easiest method is to use a function that is defined by bash-completion v2.0+, namely __load_completion. It will take care of loading the correct script if present. Afterwards, the git completion script behaves as usual. This way we can leverage bash-completion's dynamic loading for git subcommands and make it easier for developers to distribute custom completion scripts. Signed-off-by: Florian Gamböck --- contrib/completion/git-completion.bash | 8 1 file changed, 8 insertions(+) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index b09c8a236..e6114822c 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -3096,12 +3096,20 @@ __git_main () fi local completion_func="_git_${command//-/_}" + if ! declare -f $completion_func >/dev/null 2>/dev/null; then + declare -f __load_completion >/dev/null 2>/dev/null && + __load_completion "git-$command" + fi declare -f $completion_func >/dev/null 2>/dev/null && $completion_func && return local expansion=$(__git_aliased_command "$command") if [ -n "$expansion" ]; then words[1]=$expansion completion_func="_git_${expansion//-/_}" + if ! declare -f $completion_func >/dev/null 2>/dev/null; then + declare -f __load_completion >/dev/null 2>/dev/null && + __load_completion "git-$expansion" + fi declare -f $completion_func >/dev/null 2>/dev/null && $completion_func fi } -- 2.16.1
[RFC PATCH 1/1] completion: Load completion file for external subcommand
Adding external subcommands to Git is as easy as to put an executable file git-foo into PATH. Packaging such subcommands for a Linux distribution can be achieved by unpacking the executable into /usr/bin of the user's system. Adding system-wide completion scripts for new subcommands, however, can be a bit tricky. Since bash-completion started to use dynamical loading of completion scripts somewhere around v2.0, it is no longer sufficient to drop a completion script of a subcommand into the standard completions path, /usr/share/bash-completion/completions, since this script will not be loaded if called as a git subcommand. For example, look at https://bugs.gentoo.org/544722. To give a short summary: The popular git-flow subcommand provides a completion script, which gets installed as /usr/share/bash-completion/completions/git-flow. If you now type into a Bash shell: git flow You will not get any completions, because bash-completion only loads completions for git and git has no idea that git-flow is defined in another file. You have to load this script manually or trigger the dynamic loader with: git-flow # Please notice the dash instead of whitespace This will not complete anything either, because it only defines a Bash function, without generating completions. But now the correct completion script has been loaded and the first command can use the completions. So, the goal is now to teach the git completion script to consider the possibility of external completion scripts for subcommands, but of course without breaking current workflows. I think the easiest method is to use a function that is defined by bash-completion v2.0+, namely __load_completion. It will take care of loading the correct script if present. Afterwards, the git completion script behaves as usual. This way we can leverage bash-completion's dynamic loading for git subcommands and make it easier for developers to distribute custom completion scripts. Signed-off-by: Florian Gamböck --- contrib/completion/git-completion.bash | 8 1 file changed, 8 insertions(+) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index b09c8a236..e6114822c 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -3096,12 +3096,20 @@ __git_main () fi local completion_func="_git_${command//-/_}" + if ! declare -f $completion_func >/dev/null 2>/dev/null; then + declare -f __load_completion >/dev/null 2>/dev/null && + __load_completion "git-$command" + fi declare -f $completion_func >/dev/null 2>/dev/null && $completion_func && return local expansion=$(__git_aliased_command "$command") if [ -n "$expansion" ]; then words[1]=$expansion completion_func="_git_${expansion//-/_}" + if ! declare -f $completion_func >/dev/null 2>/dev/null; then + declare -f __load_completion >/dev/null 2>/dev/null && + __load_completion "git-$expansion" + fi declare -f $completion_func >/dev/null 2>/dev/null && $completion_func fi } -- 2.16.1
[RFC PATCH 0/1] completion: Dynamic completion loading
In this small patch I want to introduce a way to dynamically load completion scripts for external subcommands. A few years ago, you would put a completion script (which defines a Bash function _git_foo for a custom git subcommand foo) into /etc/bash_completion.d, or save it somewhere in your $HOME and source it manually in your .bashrc. Starting with bash-completion v2.0 (or, to be absolutely exact, the preview versions started at v1.90), completion scripts are not sourced at bash startup anymore. Rather, when typing in a command and trigger completion by pressing the TAB key, the new bash-completion's main script looks for a script with the same name as the typed command in the completion directory, sources it, and provides the completions defined in this script. However, that only works for top level commands. After a completion script has been found, the logic is delegated to this script. This means, that the completion of subcommands must be handled by the corresponding completion script. As it is now, git is perfectly able to call custom completion functions, iff they are already defined when calling the git completion. With my patch, git uses an already defined loader function of bash-completion (or continues silently if this function is not found), loads an external completion script, and provides its completions. An example for an external completion script would be /usr/share/bash-completion/completions/git-foo for a git subcommand foo. Florian Gamböck (1): completion: Load completion file for external subcommand contrib/completion/git-completion.bash | 8 1 file changed, 8 insertions(+) -- 2.16.1
Re: [RFC PATCH 1/1] completion: Load completion file for external subcommand
Ah, sorry, please ignore this one.
Re: reg. fatal: The remote end hung up unexpectedly on NFS
Avarab, I enabled GIT_TRACE, packet, etc. I could not make much sense of the output. I downloaded git source and started looking at the code. Yeah, kernel compilation with failovers worked on our filesystem. We tried xfs test suite with failovers, it worked. Since it's failing with open source NFS, I am wondering if this even has anything to do with filesystem. Jeff, Thank you for the clear instructions on strace. Looks like 5 and 6 are two ends of a pipe. read(5, "\27\3\3\5r", 5) = 5 read(5, "S\10n\230i\257\27\2\333&\335jou~\3036}S\273\212\250\33\310\216b\253\3505\332\266X"..., 1394) = 1394 write(6, "\2530\253x\27\247iz\246\321*\v\33\306.\221I\36\4\304D\\\360\361\306I\213\304\245m\247\30"..., 1370) = 1370 <... read resumed> "\2530\253x\27\247iz\246\321*\v\33\306.\221I\36\4\304D\\\360\361\306I\213\304\245m\247\30"..., 5071) = 1370 rt_sigaction(SIGPIPE, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART, 0x7f038e8bb890}, read(0, <... rt_sigaction resumed> NULL, 8) = 0 poll([{fd=5, events=POLLIN}], 1, 1000) = 1 ([{fd=5, revents=POLLIN}]) rt_sigaction(SIGPIPE, NULL, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART, 0x7f038e8bb890}, 8) = 0 rt_sigaction(SIGPIPE, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART, 0x7f038e8bb890}, NULL, 8) = 0 poll([{fd=5, events=POLLIN|POLLPRI|POLLRDNORM|POLLRDBAND}], 1, 0) = 1 ([{fd=5, revents=POLLIN|POLLRDNORM}]) read(5, "\27\3\3\5r", 5) = 5 read(5, "S\10n\230i\257\27\3]T\0{\232\253\377$\265\277\211o.\314T\315\257\335j\322\367\31o\262"..., 1394) = 1394 > Write preceding the last read has succeeded write(6, "\237%\204W$\236\177\305@\210+\236\227.\316\226\250\t\256:\27?};\270^A\317\204\222\35]"..., 1370) = 1370 <... read resumed> "\237%\204W$\236\177\305@\210+\236\227.\316\226\250\t\256:\27?};\270^A\317\204\222\35]"..., 3701) = 1370 rt_sigaction(SIGPIPE, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART, 0x7f038e8bb890}, read(0, <... rt_sigaction resumed> NULL, 8) = 0 poll([{fd=5, events=POLLIN}], 1, 1000) = 1 ([{fd=5, revents=POLLIN}]) rt_sigaction(SIGPIPE, NULL, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART, 0x7f038e8bb890}, 8) = 0 rt_sigaction(SIGPIPE, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART, 0x7f038e8bb890}, NULL, 8) = 0 poll([{fd=5, events=POLLIN|POLLPRI|POLLRDNORM|POLLRDBAND}], 1, 0) = 1 ([{fd=5, revents=POLLIN|POLLRDNORM}]) read(5, "\27\3\3\5r", 5) = 5 > Problematic reads read(5, "S\10n\230i\257\27\4\220\243\375\3772\213?\267\356V\246r\226`\223\253^\310\314\207\222\22%\376"..., 1394) = 363 read(5, "", 1031) = 0 >>> write(5, "\25\3\3\0\32\265(Nk5\330Y\4!\374S\204\377\304\0166j\rV\305;e3\347&w", 31) = 31 close(5) rt_sigaction(SIGPIPE, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART, 0x7f038e8bb890}, NULL, 8) = 0 rt_sigaction(SIGPIPE, {0x441ed0, [PIPE], SA_RESTORER|SA_RESTART, 0x7f038e8bb890}, NULL, 8) = 0 write(2, "error: RPC failed; result=18, HT"..., 46) = 46 brk(0x1597000)= 0x1597000 close(6 <... read resumed> "", 2331) = 0 <... close resumed> ) = 0 read(7, "", 4096) = 4 write(2, "fatal: The remote end hung up un"..., 43 read(7, <... write resumed> ) = 43 close(4 <... read resumed> "", 4096) = 0 <... close resumed> ) Last read from pipe (fd 5) was a partial read. Write has succeeded on the pipe but read failed, that's strange. In this case, it doesn't even look like the write was interleaved. Thanks, Satya. On Sat, Apr 7, 2018 at 1:18 AM, Jeff King wrote: > On Fri, Apr 06, 2018 at 11:55:51PM +0530, Satya Prakash GS wrote: > >> We have a distributed filesystem with NFS access. On the NFS mount, I >> was doing a git-clone and if NFS server crashed and came back up while >> the clone is going on, clone fails with the below message: >> >> git clone https://sa...@github.com/fs/private-qa.git >> >> remote: Counting objects: 139419, done. >> remote: Compressing objects: 100% (504/504), done. >> Receiving objects: 7% (9760/139419), 5.32 MiB | 5.27 MiB/s >> error: RPC failed; result=18, HTTP code = 200 MiB | 96.00 KiB/s >> fatal: The remote end hung up unexpectedly >> fatal: early EOF >> fatal: index-pack failed > > Curl's result=18 is CURLE_PARTIAL_FILE. Usually that means the other > side hung up partway through. But given the NFS symptoms you describe, I > wonder if fwrite() to the file simply returned an error, and curl gave > up. > >> On NFS server crash, it usually takes a minute or two for our >> filesystem to failover to new NFS server. Initially I suspected it had >> something to do with the filesystem, like attributes of the file >> written by git weren't matching what it was expecting but the same >> test fails on open source NFS server. While clone is going on, if I >> stopped the open source NFS server for 2 minutes and restarted it, git >> clone fails. >> >> Another interesting thing is, if the restart happens within a few >> seconds, git clone succeeds. >> >> Sideband_demux fails while trying to read from the pipe. Read size >> do
After a rebase, ORIG_HEAD points to the previous tip of the branch?
Hi, Immediately after a finished rebase, is it true that ORIG_HEAD points to the previous tip of the branch? So that `git reset --hard ORIG_HEAD` will cancel the rebase? P.S. I'm asking because most of time that is so, but there is at least one case when ORIG_HEAD breaks that assumption. Not sure if it's a bug or not. --- Best Regards, Ilya Kantor
Re: Is support for 10.8 dropped?
Hi, Eric, First of, I already have Xcode installed along with the Developer Tools. Second, here is the list of the different dylib files I found on my system: MyMac:/ igorkorot$ find . -name libSystem.B.dylib ./Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS7.1.sdk/usr/lib/libSystem.B.dylib ./Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk/usr/lib/libSystem.B.dylib ./Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/lib/libSystem.B.dylib ./usr/lib/libSystem.B.dylib I'm hoping that the dylib in the MacOSX10.9 directory does have that symbol and maybe if I can check I can export this directory and let the linker pick it up. And apparently I was wrong: MyMac:/ igorkorot$ nm -gU /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/lib/libSystem.B.dylib | grep strlcpy MyMac:/ igorkorot$ nm -gU /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk/usr/lib/libSystem.B.dylib | grep strlcpy MyMac:/ igorkorot$ I can try to build from source as this becomes my last resort. ;-) Thank you. On Sat, Apr 7, 2018 at 3:32 AM, Eric Sunshine wrote: > On Fri, Apr 6, 2018 at 10:20 PM, Igor Korot wrote: dyld: lazy symbol binding failed: Symbol not found: ___strlcpy_chk Referenced from: /usr/local/git/libexec/git-core/git Expected in: /usr/lib/libSystem.B.dylib >>> >>> It's not clear what installer you used? Was it the package from >>> git-scm? Was it from Homebrew? >> >> I just tried the git-scm installer and got exactly the same error >> during the runtime. > > Have you tried any of the suggestions at these pages for resolving this issue? > > https://stackoverflow.com/questions/22920497/git-mountain-lion-dyld-lazy-symbol-binding-failed-symbol-not-found-str > > https://stackoverflow.com/questions/20929689/git-commands-not-working-in-mac-terminal-dyld-symbol-not-found-strlcpy-ch > >>> I would guess that, even if the git-scm installer no longer supports >>> 10.8, it is likely that Homebrew does. Have you tried it? >> >> I don't want to pollute my system with Homebrew. >> >>> If both those options fail, you can always build from source. >> >> Where do I get the soure code? And how do I build it? >> I guess I have only one option left. ;-) > > Source code for the latest release is at: > https://github.com/git/git/archive/v2.17.0.zip > > To build it, you'll need to have the MacOS Developer Tools installed. > It's also quite likely that you'll need to build some prerequisite > libraries; at minimum OpenSSL. You may be able to skip other libraries > if you don't care about the functionality. Build settings which you > may need to adjust to disable dependence on libraries in which you're > not interested are documented at the top of Makefile. Place overrides > for those settings in a file named config.mak in the git directory.
Re: [PATCH v12 4/4] ls-remote: create '--sort' option
Harald Nordgren writes: > +--sort=:: > + Sort based on the key given. Prefix `-` to sort in descending order > + of the value. Supports "version:refname" or "v:refname" (tag names > + are treated as versions). The "version:refname" sort order can also > + be affected by the "versionsort.suffix" configuration variable. > + See linkgit:git-for-each-ref[1] for more sort options, but be aware > + that because `ls-remote` deals only with remotes, any key like > + `committerdate` that requires access to the object itself will > + cause a failure. I can connect "because it deals only with remotes" and "access to the object may fail", but I wonder if we can clarify the former a bit better to make it easier to understand for those who are not yet familiar with Git. Keys like `committerdate` that require access to the object will not work [***HOW??? Do we get a segfault or something???***] for refs whose objects have not yet been fetched from the remote. I added "for refs whose objects have not yet been fetched", whose explicit-ness made "will not work" to be an OK expression, but without it I would have suggested to rephrase it to "may not work". This is a tangent but I suspect that the description of --sort in git-for-each-ref documentation is wrong on the use of multiple options, or I am misreading parse_opt_ref_sorting(), which I think appends later keys to the end of the list, and compare_refs() which I think yields results when an earlier key in the last decides two are not equal and ignores the later keys. The commit that added the description was c0f6dc9b ("git-for-each-ref.txt: minor improvements", 2008-06-05), and I think even back then the code in builtin-for-each-ref.c appended later keys at the end, and it seems nobody complained, so it is possible I am not reading the code right before fully adjusting to timezone change ;-) > + for (i = 0; i < ref_array.nr; i++) { > + const struct ref_array_item *ref = ref_array.items[i]; > if (show_symref_target && ref->symref) > - printf("ref: %s\t%s\n", ref->symref, ref->name); > - printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name); > + printf("ref: %s\t%s\n", ref->symref, ref->refname); > + printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname); > status = 0; /* we found something */ > } > + > + UNLEAK(sorting); > + UNLEAK(ref_array); > return status; > } It is not too big a deal, but I find that liberal sprinkling of UNLEAK() is somewhat unsightly. From the code we leave with this change, it is clear what we'd need to do when we make the code fully leak-proof (i.e. we'd have a several lines to free resources held by these two variables here, and then UNLEAK() that appear earlier in the function will become a "goto cleanup;" after setting appropriate value to "status"), so let's not worry about it. > +test_expect_success 'ls-remote --sort="version:refname" --tags self' ' > + cat >expect <<-\EOF && > + '$(git rev-parse mark)' refs/tags/mark > + '$(git rev-parse mark1.1)' refs/tags/mark1.1 > + '$(git rev-parse mark1.2)' refs/tags/mark1.2 > + '$(git rev-parse mark1.10)' refs/tags/mark1.10 > + EOF Please do *NOT* step outside the pair of single quotes that protects the body of the test scriptlet and execute commands like "git rev-parse". These execute in order to prepare the command line argument strings BEFORE test_expect_success can run (or the test framework decides if the test will be skipped via GIT_TEST_SKIP). Instead do it like so: cat >expect <<-EOF && $(git rev-parse mark) refs/tags/mark $(git rev-parse mark1.1)refs/tags/mark1.1 $(git rev-parse mark1.2)refs/tags/mark1.2 $(git rev-parse mark1.10) refs/tags/mark1.10 EOF I.e. the end token for << that is not quoted allows $var and $(cmd) to be substituted. Same comment applies throughout the remainder of this file. Other than that, this patch was a very pleasant read. Thanks.
Re: Is support for 10.8 dropped?
Eric, Sorry for multiple e-mails. I'm just trying to go thru with this. This is what I tried: [code] MyMac:git-2.17.0 igorkorot$ make configure GIT_VERSION = 2.17.0 GEN configure /bin/sh: autoconf: command not found make: *** [configure] Error 127 MyMac:git-2.17.0 igorkorot$ [/code] Does this mean that something is a miss? Thank you. On Sun, Apr 8, 2018 at 5:10 PM, Igor Korot wrote: > Hi, Eric, > First of, I already have Xcode installed along with the Developer Tools. > Second, here is the list of the different dylib files I found on my system: > > MyMac:/ igorkorot$ find . -name libSystem.B.dylib > ./Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS7.1.sdk/usr/lib/libSystem.B.dylib > ./Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk/usr/lib/libSystem.B.dylib > ./Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/lib/libSystem.B.dylib > ./usr/lib/libSystem.B.dylib > > I'm hoping that the dylib in the MacOSX10.9 directory does have that > symbol and maybe if I can check I can export this directory and let > the linker pick it up. > > And apparently I was wrong: > > MyMac:/ igorkorot$ nm -gU > /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/lib/libSystem.B.dylib > | grep strlcpy > MyMac:/ igorkorot$ nm -gU > /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk/usr/lib/libSystem.B.dylib > | grep strlcpy > MyMac:/ igorkorot$ > > I can try to build from source as this becomes my last resort. ;-) > > Thank you. > > > On Sat, Apr 7, 2018 at 3:32 AM, Eric Sunshine wrote: >> On Fri, Apr 6, 2018 at 10:20 PM, Igor Korot wrote: > dyld: lazy symbol binding failed: Symbol not found: ___strlcpy_chk > Referenced from: /usr/local/git/libexec/git-core/git > Expected in: /usr/lib/libSystem.B.dylib It's not clear what installer you used? Was it the package from git-scm? Was it from Homebrew? >>> >>> I just tried the git-scm installer and got exactly the same error >>> during the runtime. >> >> Have you tried any of the suggestions at these pages for resolving this >> issue? >> >> https://stackoverflow.com/questions/22920497/git-mountain-lion-dyld-lazy-symbol-binding-failed-symbol-not-found-str >> >> https://stackoverflow.com/questions/20929689/git-commands-not-working-in-mac-terminal-dyld-symbol-not-found-strlcpy-ch >> I would guess that, even if the git-scm installer no longer supports 10.8, it is likely that Homebrew does. Have you tried it? >>> >>> I don't want to pollute my system with Homebrew. >>> If both those options fail, you can always build from source. >>> >>> Where do I get the soure code? And how do I build it? >>> I guess I have only one option left. ;-) >> >> Source code for the latest release is at: >> https://github.com/git/git/archive/v2.17.0.zip >> >> To build it, you'll need to have the MacOS Developer Tools installed. >> It's also quite likely that you'll need to build some prerequisite >> libraries; at minimum OpenSSL. You may be able to skip other libraries >> if you don't care about the functionality. Build settings which you >> may need to adjust to disable dependence on libraries in which you're >> not interested are documented at the top of Makefile. Place overrides >> for those settings in a file named config.mak in the git directory.
Re: [RFC PATCH 1/1] completion: load completion file for external subcommand
Florian Gamböck writes: > diff --git a/contrib/completion/git-completion.bash > b/contrib/completion/git-completion.bash > index b09c8a236..e6114822c 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -3096,12 +3096,20 @@ __git_main () > fi > Sorry if I am asking something obvious, as I am not fluent in bash-isms, but > local completion_func="_git_${command//-/_}" > + if ! declare -f $completion_func >/dev/null 2>/dev/null; then > + declare -f __load_completion >/dev/null 2>/dev/null && > + __load_completion "git-$command" wouldn't the above be easier to read if it were if ! declare ... $completion_func ... && declare -f __load_completion then __load_completion "git-$command" fi or is there a reason why it is better to &&-chain the check for __load_completion with its use? Same comment applies to the other hunk.
Re: After a rebase, ORIG_HEAD points to the previous tip of the branch?
Ilya Kantor writes: > Immediately after a finished rebase, is it true that ORIG_HEAD points > to the previous tip of the branch? > > So that `git reset --hard ORIG_HEAD` will cancel the rebase? I wouldn't recommend people to depend on it; rather use "@{1}", perhaps? Before reflog was invented, ORIG_HEAD was primarily a handy way to back out of "reset", but because the implementation of higher-level porcelains like "rebase" may internally use things like "reset" without giving it much thought about clobber ingORIG_HEAD (largely because there is an expectation that modern end users would be using reflog more than old-way ORIG_HEAD), cases like stopping in conflict and getting told to skip and continue may not preserve ORIG_HEAD as you wish it to.
Re: [PATCH v4] filter-branch: fix errors caused by refs that point at non-committish
Yuki Kokubun writes: > References: <1521996898-7052-1-git-send-email-orga.chem@gmail.com> > Content-Type: text/plain > > Sorry, I forgot add a line of "Reviewed-by". > I'm gonna send the fixed patch again. Do not worry too much about this. Reviewed-by: added by patch author is seen rather rarely, because the following sequence of events need to happen: - The author sends a patch vN - A reviewer reviews vN and says "This is now Reviewed-by: me" - The author re-submits patch vN+1, where the only difference between vN and vN+1 is essentially _nothing_. Except for the addition of "Reviewed-by:" by the reviewer. But our typical patch injestion cycle is much faster and often the last step does not happen ;-)
Re: [PATCH 1/9] git_config_set: fix off-by-two
Johannes Schindelin writes: >> Yes, it is a workaround. Making shell faster on windows would of >> course be one possible solution to make t/t*.sh scripts go faster >> ;-) Or update parts of t/t*.sh so that the equivalent test coverage >> can be kept while running making them go faster on Windows. > > What makes you think that I did not try my hardest for around 812 hours in > total so far to make the shell faster? Nowhere in these four lines I ever said that I think you did not work hard to solve the performance issues you have.
Re: [PATCH] specify encoding for sed command
Stephon Harris writes: > Fixes issue with seeing `sed: RE error: illegal byte sequence` when running > git-completion.bash > --- > contrib/completion/git-completion.bash | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/contrib/completion/git-completion.bash > b/contrib/completion/git-completion.bash > index b09c8a23626b4..52a4ab5e2165a 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -282,7 +282,7 @@ __gitcomp () > > # Clear the variables caching builtins' options when (re-)sourcing > # the completion script. > -unset $(set |sed -ne > 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null > +unset $(set |LANG=C sed -ne > 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null Shouldn't LC_ALL and LANG both set and exported to C, as (1) ancient systems understand only LANG but not LC_*; and (2) modern ones that understand both give precedence to LC_ALL over LANG? If we were to set only one, it is probably more sensible to set LC_ALL, I suspect, but it may be better to set both, which sends a sign to the readers that we know what we are doing ;-)
Re: [PATCH v4 1/3] builtin/config: introduce `--default`
Jeff King writes: > On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote: > >> @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char >> *regex_) >> config_with_options(collect_config, &values, >> &given_config_source, &config_options); >> >> +if (!values.nr && default_value) { >> +struct strbuf *item; >> +ALLOC_GROW(values.items, values.nr + 1, values.alloc); >> +item = &values.items[values.nr++]; >> +strbuf_init(item, 0); >> +if (format_config(item, key_, default_value) < 0) { >> +exit(1); >> +} >> +} > ... > > It's obvious in this toy example, but that config call may be buried > deep in a script. It'd probably be nicer for that exit(1) to be > something like: > > die(_("failed to format default config value")); Together with key_ and default_value, you mean? > >> +test_expect_success 'does not allow --default without --get' ' >> +test_must_fail git config --default quux --unset a >output 2>&1 && >> +test_i18ngrep "\-\-default is only applicable to" output >> +' > > I think "a" here needs to be "a.section". I get: > > error: key does not contain a section: a "section.var"? That aside, even with a properly formatted key, I seem to get an empty output here, so this may need a bit more work.
Re: [PATCH 3/3] ref-filter: factor ref_array pushing into its own function
Jeff King writes: > In preparation for callers constructing their own ref_array > structs, let's move our own internal push operation into its > own function. > > While we're at it, we can replace REALLOC_ARRAY() with > ALLOC_GROW(), which should give the growth operation > amortized linear complexity (as opposed to growing by one, > which is potentially quadratic, though in-place realloc > growth often makes this faster in practice). Sorry, but I do not quite get the last part this paragraph. Up to but not including ", though in-place..." I would understand as: - With ALLOC_GROW(), we are explicit in that we are amortizing the allocation cost by growing in exponential batches. - With REALLOC_ARRAY(), we are telling the underlying realloc(3) that it is OK to pretend that we grow by one, but if that permission is literally followed, it could end up quadratic. So if you have really a bad realloc(3) implementation, switching to use ALLOC_GROW() is an improvement. But after that I am not sure what you are getting at. Do you mean "In practice, a well-done realloc(3) does the amortizing internally anyway, which makes it faster than the grow-by-one quadratic, so this change may not make that much of a difference"? Or do you mean "In practice, a well-done realloc(3) internally amortizes better than we do manually, so this change may hurt performance"? In either case, I think this splitting into a helper is obviously a good move, and using ALLOC_GROW() is conceptually cleaner, as we use it everywhere and tend to avoid realloc(3) just to add one item. Other than that small confusion (not even a nit), three patches were pleasant read. Thanks.
Re: [PATCH v2 0/4] Lazy-load trees when reading commit-graph
Jeff King writes: > If I were doing it myself, I probably would have folded patches 1 and 3 > together. They are touching all the same spots, and it would be an error > for any case converted in patch 1 to not get converted in patch 3. I'm > assuming you caught them all due to Coccinelle, though IMHO it is > somewhat overkill here. By folding them together the compiler could tell > you which spots you missed. Yeah, that approach would probably be a more sensible way to assure the safety/correctness of the result to readers better. > > And going forward, I doubt it is going to be a common error for people > to use maybe_tree directly. Between the name and the warning comment, > you'd have to really try to shoot yourself in the foot with it. The > primary concern was catching people using the existing "tree" name, > whose semantics changed. Yup.
Re: Is support for 10.8 dropped?
On Sun, Apr 8, 2018 at 6:37 PM, Igor Korot wrote: > MyMac:git-2.17.0 igorkorot$ make configure > GIT_VERSION = 2.17.0 > GEN configure > /bin/sh: autoconf: command not found > make: *** [configure] Error 127 > > Does this mean that something is a miss? That means that you don't have the Autoconf tools installed, but don't worry about it since use of the 'configure' script it very optional in this project. Git tends to build just fine on most platforms without running 'configure'. You should be able to get by just by typing "make" in the git directory after downloading the source. And, as noted earlier, before running "make", you may need to create config.mak to override some settings documented at the top of Makefile (in particular, you may want to set NO_GETTEXT if you don't want to install gettext and don't think you'll need it). As prerequisite, you'll probably need to install OpenSSL.
[PATCH v13 2/4] ref-filter: make ref_array_item allocation more consistent
From: Jeff King We have a helper function to allocate ref_array_item structs, but it only takes a subset of the possible fields in the struct as initializers. We could have it accept an argument for _every_ field, but that becomes a pain for the fields which some callers don't want to set initially. Instead, let's be explicit that it takes only the minimum required to create the ref, and that callers should then fill in the rest themselves. Signed-off-by: Jeff King Signed-off-by: Harald Nordgren --- ref-filter.c | 18 +++--- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index ade97a848..c1c3cc948 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1824,15 +1824,18 @@ static const struct object_id *match_points_at(struct oid_array *points_at, return NULL; } -/* Allocate space for a new ref_array_item and copy the objectname and flag to it */ +/* + * Allocate space for a new ref_array_item and copy the name and oid to it. + * + * Callers can then fill in other struct members at their leisure. + */ static struct ref_array_item *new_ref_array_item(const char *refname, -const struct object_id *oid, -int flag) +const struct object_id *oid) { struct ref_array_item *ref; + FLEX_ALLOC_STR(ref, refname, refname); oidcpy(&ref->objectname, oid); - ref->flag = flag; return ref; } @@ -1927,12 +1930,13 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid, * to do its job and the resulting list may yet to be pruned * by maxcount logic. */ - ref = new_ref_array_item(refname, oid, flag); + ref = new_ref_array_item(refname, oid); ref->commit = commit; + ref->flag = flag; + ref->kind = kind; REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1); ref_cbdata->array->items[ref_cbdata->array->nr++] = ref; - ref->kind = kind; return 0; } @@ -2169,7 +2173,7 @@ void pretty_print_ref(const char *name, const struct object_id *oid, const struct ref_format *format) { struct ref_array_item *ref_item; - ref_item = new_ref_array_item(name, oid, 0); + ref_item = new_ref_array_item(name, oid); ref_item->kind = ref_kind_from_refname(name); show_ref_array_item(ref_item, format); free_array_item(ref_item); -- 2.14.3 (Apple Git-98)
[PATCH v13 4/4] ls-remote: create '--sort' option
Create a '--sort' option for ls-remote, based on the one from for-each-ref. This e.g. allows ref names to be sorted by version semantics, so that v1.2 is sorted before v1.10. Signed-off-by: Harald Nordgren --- Notes: Changes according to Junio Hamano's code review Documentation/git-ls-remote.txt | 17 - builtin/ls-remote.c | 25 +-- t/t5512-ls-remote.sh| 54 +++-- 3 files changed, 86 insertions(+), 10 deletions(-) diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt index 5f2628c8f..0e26a67a1 100644 --- a/Documentation/git-ls-remote.txt +++ b/Documentation/git-ls-remote.txt @@ -10,7 +10,7 @@ SYNOPSIS [verse] 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=] - [-q | --quiet] [--exit-code] [--get-url] + [-q | --quiet] [--exit-code] [--get-url] [--sort=] [--symref] [ [...]] DESCRIPTION @@ -60,6 +60,17 @@ OPTIONS upload-pack only shows the symref HEAD, so it will be the only one shown by ls-remote. +--sort=:: + Sort based on the key given. Prefix `-` to sort in descending order + of the value. Supports "version:refname" or "v:refname" (tag names + are treated as versions). The "version:refname" sort order can also + be affected by the "versionsort.suffix" configuration variable. + See linkgit:git-for-each-ref[1] for more sort options, but be aware + that because `ls-remote` deals only with remotes, keys like + `committerdate` that requires access to the objects themselves will + not work for refs whose objects have not yet been fetched from the + remote. + :: The "remote" repository to query. This parameter can be either a URL or the name of a remote (see the GIT URLS and @@ -90,6 +101,10 @@ EXAMPLES c5db5456ae3b0873fc659c19fafdde22313cc441refs/tags/v0.99.2 7ceca275d047c90c0c7d5afb13ab97efdf51bd6erefs/tags/v0.99.3 +SEE ALSO + +linkgit:git-check-ref-format[1]. + GIT --- Part of the linkgit:git[1] suite diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c index 540d56429..b5ca67167 100644 --- a/builtin/ls-remote.c +++ b/builtin/ls-remote.c @@ -1,6 +1,7 @@ #include "builtin.h" #include "cache.h" #include "transport.h" +#include "ref-filter.h" #include "remote.h" static const char * const ls_remote_usage[] = { @@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) int show_symref_target = 0; const char *uploadpack = NULL; const char **pattern = NULL; + int i; struct remote *remote; struct transport *transport; const struct ref *ref; + struct ref_array ref_array; + static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting; struct option options[] = { OPT__QUIET(&quiet, N_("do not print remote URL")), @@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL), OPT_BOOL(0, "get-url", &get_url, N_("take url..insteadOf into account")), + OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"), +N_("field name to sort on"), &parse_opt_ref_sorting), OPT_SET_INT_F(0, "exit-code", &status, N_("exit with exit code 2 if no matching refs are found"), 2, PARSE_OPT_NOCOMPLETE), @@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) OPT_END() }; + memset(&ref_array, 0, sizeof(ref_array)); + argc = parse_options(argc, argv, prefix, options, ls_remote_usage, PARSE_OPT_STOP_AT_NON_OPTION); dest = argv[0]; @@ -104,14 +112,27 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) if (!dest && !quiet) fprintf(stderr, "From %s\n", *remote->url); for ( ; ref; ref = ref->next) { + struct ref_array_item *item; if (!check_ref_type(ref, flags)) continue; if (!tail_match(pattern, ref->name)) continue; + item = ref_array_push(&ref_array, ref->name, &ref->old_oid); + item->symref = xstrdup_or_null(ref->symref); + } + + if (sorting) + ref_array_sort(sorting, &ref_array); + + for (i = 0; i < ref_array.nr; i++) { + const struct ref_array_item *ref = ref_array.items[i]; if (show_symref_target && ref->symref) - printf("ref: %s\t%s\n", ref->symref, ref->name); - printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name); + printf("ref: %
[PATCH v13 3/4] ref-filter: factor ref_array pushing into its own function
From: Jeff King In preparation for callers constructing their own ref_array structs, let's move our own internal push operation into its own function. While we're at it, we can replace REALLOC_ARRAY() with ALLOC_GROW(), which should give the growth operation amortized linear complexity (as opposed to growing by one, which is potentially quadratic, though in-place realloc growth often makes this faster in practice). Signed-off-by: Jeff King Signed-off-by: Harald Nordgren --- ref-filter.c | 16 +--- ref-filter.h | 8 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index c1c3cc948..6e9328b27 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1840,6 +1840,18 @@ static struct ref_array_item *new_ref_array_item(const char *refname, return ref; } +struct ref_array_item *ref_array_push(struct ref_array *array, + const char *refname, + const struct object_id *oid) +{ + struct ref_array_item *ref = new_ref_array_item(refname, oid); + + ALLOC_GROW(array->items, array->nr + 1, array->alloc); + array->items[array->nr++] = ref; + + return ref; +} + static int ref_kind_from_refname(const char *refname) { unsigned int i; @@ -1930,13 +1942,11 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid, * to do its job and the resulting list may yet to be pruned * by maxcount logic. */ - ref = new_ref_array_item(refname, oid); + ref = ref_array_push(ref_cbdata->array, refname, oid); ref->commit = commit; ref->flag = flag; ref->kind = kind; - REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1); - ref_cbdata->array->items[ref_cbdata->array->nr++] = ref; return 0; } diff --git a/ref-filter.h b/ref-filter.h index 68268f9eb..76cf87cb6 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -135,4 +135,12 @@ void setup_ref_filter_porcelain_msg(void); void pretty_print_ref(const char *name, const struct object_id *oid, const struct ref_format *format); +/* + * Push a single ref onto the array; this can be used to construct your own + * ref_array without using filter_refs(). + */ +struct ref_array_item *ref_array_push(struct ref_array *array, + const char *refname, + const struct object_id *oid); + #endif /* REF_FILTER_H */ -- 2.14.3 (Apple Git-98)
[PATCH v13 1/4] ref-filter: use "struct object_id" consistently
From: Jeff King Internally we store a "struct object_id", and all of our callers have one to pass us. But we insist that they peel it to its bare-sha1 hash, which we then hashcpy() into place. Let's pass it around as an object_id, which future-proofs us for a post-sha1 world. Signed-off-by: Jeff King Signed-off-by: Harald Nordgren --- builtin/tag.c| 2 +- builtin/verify-tag.c | 2 +- ref-filter.c | 10 +- ref-filter.h | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index da186691e..42278f516 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -117,7 +117,7 @@ static int verify_tag(const char *name, const char *ref, return -1; if (format->format) - pretty_print_ref(name, oid->hash, format); + pretty_print_ref(name, oid, format); return 0; } diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index ad7b79fa5..6fa04b751 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -72,7 +72,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) } if (format.format) - pretty_print_ref(name, oid.hash, &format); + pretty_print_ref(name, &oid, &format); } return had_error; } diff --git a/ref-filter.c b/ref-filter.c index 45fc56216..ade97a848 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1826,12 +1826,12 @@ static const struct object_id *match_points_at(struct oid_array *points_at, /* Allocate space for a new ref_array_item and copy the objectname and flag to it */ static struct ref_array_item *new_ref_array_item(const char *refname, -const unsigned char *objectname, +const struct object_id *oid, int flag) { struct ref_array_item *ref; FLEX_ALLOC_STR(ref, refname, refname); - hashcpy(ref->objectname.hash, objectname); + oidcpy(&ref->objectname, oid); ref->flag = flag; return ref; @@ -1927,7 +1927,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid, * to do its job and the resulting list may yet to be pruned * by maxcount logic. */ - ref = new_ref_array_item(refname, oid->hash, flag); + ref = new_ref_array_item(refname, oid, flag); ref->commit = commit; REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1); @@ -2165,11 +2165,11 @@ void show_ref_array_item(struct ref_array_item *info, putchar('\n'); } -void pretty_print_ref(const char *name, const unsigned char *sha1, +void pretty_print_ref(const char *name, const struct object_id *oid, const struct ref_format *format) { struct ref_array_item *ref_item; - ref_item = new_ref_array_item(name, sha1, 0); + ref_item = new_ref_array_item(name, oid, 0); ref_item->kind = ref_kind_from_refname(name); show_ref_array_item(ref_item, format); free_array_item(ref_item); diff --git a/ref-filter.h b/ref-filter.h index 0d98342b3..68268f9eb 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -132,7 +132,7 @@ void setup_ref_filter_porcelain_msg(void); * Print a single ref, outside of any ref-filter. Note that the * name must be a fully qualified refname. */ -void pretty_print_ref(const char *name, const unsigned char *sha1, +void pretty_print_ref(const char *name, const struct object_id *oid, const struct ref_format *format); #endif /* REF_FILTER_H */ -- 2.14.3 (Apple Git-98)
Re: Is support for 10.8 dropped?
On Sun, Apr 8, 2018 at 7:55 PM, Igor Korot wrote: > On Sun, Apr 8, 2018, 6:23 PM Eric Sunshine wrote: >> And, as noted earlier, before running "make", you may need to create >> config.mak to override some settings documented at the top of Makefile >> (in particular, you may want to set NO_GETTEXT if you don't want to >> install gettext and don't think you'll need it). As prerequisite, >> you'll probably need to install OpenSSL. > > Is there a way to check for OpenSSL presence? Not sure what you're asking. Are you asking how to determine if you already have OpenSSL built on your machine? Note that you might be able to get by without installing OpenSSL since Git will try to use Apple's "Common Crypto" instead, so you could define NO_OPENSSL in config.mak and see if the project builds.
Re: [PATCH v12 4/4] ls-remote: create '--sort' option
On Mon, Apr 9, 2018 at 12:16 AM, Junio C Hamano wrote: > > I can connect "because it deals only with remotes" and "access to > the object may fail", but I wonder if we can clarify the former a > bit better to make it easier to understand for those who are not yet > familiar with Git. > > Keys like `committerdate` that require access to the object will > not work [***HOW??? Do we get a segfault or something???***] for > refs whose objects have not yet been fetched from the remote. > > I added "for refs whose objects have not yet been fetched", whose > explicit-ness made "will not work" to be an OK expression, but > without it I would have suggested to rephrase it to "may not work". > > This is a tangent but I suspect that the description of --sort in > git-for-each-ref documentation is wrong on the use of multiple > options, or I am misreading parse_opt_ref_sorting(), which I think > appends later keys to the end of the list, and compare_refs() which > I think yields results when an earlier key in the last decides two > are not equal and ignores the later keys. The commit that added the > description was c0f6dc9b ("git-for-each-ref.txt: minor > improvements", 2008-06-05), and I think even back then the code in > builtin-for-each-ref.c appended later keys at the end, and it seems > nobody complained, so it is possible I am not reading the code right > before fully adjusting to timezone change ;-) Updated the docs. > > It is not too big a deal, but I find that liberal sprinkling of > UNLEAK() is somewhat unsightly. From the code we leave with this > change, it is clear what we'd need to do when we make the code fully > leak-proof (i.e. we'd have a several lines to free resources held by > these two variables here, and then UNLEAK() that appear earlier in > the function will become a "goto cleanup;" after setting appropriate > value to "status"), so let's not worry about it. Removed the "extra" unleak annotations within the if-return blocks, but kept them at the end. > Please do *NOT* step outside the pair of single quotes that protects > the body of the test scriptlet and execute commands like "git > rev-parse". These execute in order to prepare the command line > argument strings BEFORE test_expect_success can run (or the test > framework decides if the test will be skipped via GIT_TEST_SKIP). > > Instead do it like so: > > cat >expect <<-EOF && > $(git rev-parse mark) refs/tags/mark > $(git rev-parse mark1.1)refs/tags/mark1.1 > $(git rev-parse mark1.2)refs/tags/mark1.2 > $(git rev-parse mark1.10) refs/tags/mark1.10 > EOF > > I.e. the end token for << that is not quoted allows $var and $(cmd) > to be substituted. > > Same comment applies throughout the remainder of this file. Ah, thanks! I was looking for some way to allow the values to expand within the proper test context. So turns out '<<-\EOF' vs '<<-EOF' makes all the difference :) > Other than that, this patch was a very pleasant read. Thanks. Thank you!
Re: [PATCH v6 6.5/6] fixup! worktree: teach "add" to check out existing branches
On Sun, Apr 1, 2018 at 9:11 AM, Thomas Gummerer wrote: > So while playing with it a bit more I found one case where the new UI > is not ideal and a bit confusing. Namely when the new check out dwim > kicks in, but there is already a file/directory at the path we're > giving to 'git worktree add'. > > In that case something like the following would be printed: > > $ g worktree add ../next > Checking out branch 'next' > fatal: '../next' already exists > > Instead I think we'd just want the error without the "Checking out > branch" message, which is what this fixup here does. Doesn't the same UI "problem" exist when it creates a new branch? $ git worktree add ../dwim Creating branch 'dwim' fatal: '../dwim' already exists As you mention below, we don't (yet) clean up the newly-created branch upon failure, so we can't suppress the "Creating branch" message as you suppress the "Checking out branch" message above (since the user needs to know that the new branch exists). This is making me wonder if "Checking out branch" is perhaps the wrong terminology. What if it said something like this instead: $ git worktree add ../next Preparing worktree (branch 'next' <= 'origin/next') fatal: '../next' already exists $ git worktree add ../gobble Preparing worktree (new branch 'gobble') fatal: '../gobble' already exists This way, we don't need the special case added by this "fixup!" patch. (I'm not wedded to the "Preparing" message but just used it as an example; better suggestions welcome.) > One thing that gets a bit strange is that the "Checking out branch" > message and the "Creating branch" messages are printed from different > places. But without doing quite some refactoring I don't think > there's a good way to do that, and I think having the UI do the right > thing is more important. The implementation is getting rather ugly, though, especially with these messages being printed by different bits of code like this. worktree.c:add_worktree() was supposed to be the low-level worker; it wasn't intended for it to take on UI duties like this "fixup!" makes it do. UI should be handled by worktree.c:add(). Taking the above "Preparing..." idea into consideration, then it should be possible to sidestep this implementation ugliness, I would think. > One thing I also noticed is that if a branch is created by 'git > worktree add', but we fail, we never clean up that branch again, which > I'm not sure is ideal. As a pre-existing problem I'd like to keep > fixing that out of the scope of this series though (at least after > this series the user would get some output showing that this happened, > even when the branch is not set up to track a remote), so I'd like to > keep fixing that out of the scope of this series. Nice idea, but outside the scope of this series, as you mention. > diff --git a/builtin/worktree.c b/builtin/worktree.c > @@ -27,6 +27,7 @@ struct add_opts { > int keep_locked; > + int checkout_existing_branch; > }; > @@ -316,6 +317,8 @@ static int add_worktree(const char *path, const char > *refname, > + if (opts->checkout_existing_branch) > + fprintf_ln(stderr, _("Checking out branch '%s'"), refname); > if (opts->checkout) { I'd have expected to see the "if (opts->checkout_existing_branch) fprintf_ln(...)" inside the following "if (opts->checkout)" conditional, though, as noted above, I'm not entirely happy about worktree.c:add_worktree() taking on UI duties. > cp.argv = NULL; > argv_array_clear(&cp.args);
Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
On Sun, Apr 8, 2018 at 10:24 AM, Thomas Gummerer wrote: > On 04/08, Eric Sunshine wrote: >> On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer >> wrote: > Let me think through some of the cases here, of 'git worktre add > ' with various flags and what the UI would be with > that added: > > - no flags: > > $ git worktree add ../test origin/master > Checking out 'origin/master' > Checking out files: ...% > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone' > > - -b branch: > > $ git worktree add -b test ../test origin/master > Creating branch 'test' > Checking out 'origin/master' Did you mean "Checking out 'test'"? > Checking out files: ...% > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone' > > Would we want to omit the "Checking out ..." here? I'm leaning > towards yes, but dunno? To which "Checking out" message do you refer, the one showing the branch name or the one showing the checkout progress? I'd probably agree that showing both "Creating" and "Checkout out" is overkill. However, see my response[1] to your "fixup!" patch in which I explore the idea that unifying "Checking out 'branch' and "Creating branch" messages may be a good idea and get us out of some UI jams which seem to be cropping up. [1]: https://public-inbox.org/git/20180325134947.25828-1-t.gumme...@gmail.com/T/#m5d38b0c6427609e8c36aa6af83d518791c1e1581 > - Original dwim with --detach flag > > $ git worktree add --detach ../test > Checking out 'c2a499e6c' > Checking out files: ...% > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone' > > Looking at this, I'm not sure what's best here. I'm not sure I'm a > fan of the duplicate "Checking out " message (I assume that's what you > meant above, or did you mean just "Checkout ..."?) Taking [1] into account, this might become something like: $ git worktree add --detach ../test Preparing worktree (detached HEAD c2a499e6c) Checking out files: ...% New worktree HEAD is now at c2a499e6c Gobbledygook > I als don't think it gives too much context compared to just "Checking > out files: ...%". I think it gives a bit more context when that > message is not displayed at all, as it shows whether files are checked > out or not, but if we do that, when we create a new branch, the amount > of output we'd display is getting a bit long, to the point where I > suspect users would just not read it anymore. > > So I personally don't feel like this is worth it, even though it may > give some context in some cases. Fair enough observation. The idea suggested in [1] may keep output to a reasonable amount.
Re: [PATCH v12 4/4] ls-remote: create '--sort' option
Harald Nordgren writes: >> It is not too big a deal, but I find that liberal sprinkling of >> UNLEAK() is somewhat unsightly. From the code we leave with this >> change, it is clear what we'd need to do when we make the code fully >> leak-proof (i.e. we'd have a several lines to free resources held by >> these two variables here, and then UNLEAK() that appear earlier in >> the function will become a "goto cleanup;" after setting appropriate >> value to "status"), so let's not worry about it. > > Removed the "extra" unleak annotations within the if-return blocks, > but kept them at the end. I actually think that is worse. The UNLEAK()s near early-return serve to remind us: "here we are making early return and when we properly plug the leak, here is one of the places we need to fix". Please keep them, unless (and I do not recommend to do this) you are plugging the leaks for real.
Re: [PATCH v13 4/4] ls-remote: create '--sort' option
Harald Nordgren writes: > Create a '--sort' option for ls-remote, based on the one from > for-each-ref. This e.g. allows ref names to be sorted by version > semantics, so that v1.2 is sorted before v1.10. > > Signed-off-by: Harald Nordgren > --- Thanks. > +--sort=:: > + Sort based on the key given. Prefix `-` to sort in descending order > + of the value. Supports "version:refname" or "v:refname" (tag names > + are treated as versions). The "version:refname" sort order can also > + be affected by the "versionsort.suffix" configuration variable. > + See linkgit:git-for-each-ref[1] for more sort options, but be aware > + that because `ls-remote` deals only with remotes, keys like > + `committerdate` that requires access to the objects themselves will > + not work for refs whose objects have not yet been fetched from the > + remote. With the update since v12, I think "because `ls-remote` deals only with remotes," can be dropped entirely, and still convey what needs to be told: "Be aware some keys that needs access to objects that are not here won't work". Instead, it is probably a better idea to spend that half-line worth of characters to describe in what way they do not work (do they try to deref a NULL pointer and dump core? do they notice we need missing objects and give an error?). > diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c > index 540d56429..b5ca67167 100644 > --- a/builtin/ls-remote.c > +++ b/builtin/ls-remote.c As I said earlier, let's keep these extra UNLEAK() near early return; they point developers at the places that needs future work. Thanks. diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c index b5ca67167d..d3851074c2 100644 --- a/builtin/ls-remote.c +++ b/builtin/ls-remote.c @@ -98,6 +98,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) if (get_url) { printf("%s\n", *remote->url); + UNLEAK(sorting); return 0; } @@ -106,8 +107,10 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack); ref = transport_get_remote_refs(transport); - if (transport_disconnect(transport)) + if (transport_disconnect(transport)) { + UNLEAK(sorting); return 1; + } if (!dest && !quiet) fprintf(stderr, "From %s\n", *remote->url);
[PATCH v14 1/4] ref-filter: use "struct object_id" consistently
From: Jeff King Internally we store a "struct object_id", and all of our callers have one to pass us. But we insist that they peel it to its bare-sha1 hash, which we then hashcpy() into place. Let's pass it around as an object_id, which future-proofs us for a post-sha1 world. Signed-off-by: Jeff King Signed-off-by: Harald Nordgren --- builtin/tag.c| 2 +- builtin/verify-tag.c | 2 +- ref-filter.c | 10 +- ref-filter.h | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index da186691e..42278f516 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -117,7 +117,7 @@ static int verify_tag(const char *name, const char *ref, return -1; if (format->format) - pretty_print_ref(name, oid->hash, format); + pretty_print_ref(name, oid, format); return 0; } diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index ad7b79fa5..6fa04b751 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -72,7 +72,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) } if (format.format) - pretty_print_ref(name, oid.hash, &format); + pretty_print_ref(name, &oid, &format); } return had_error; } diff --git a/ref-filter.c b/ref-filter.c index 45fc56216..ade97a848 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1826,12 +1826,12 @@ static const struct object_id *match_points_at(struct oid_array *points_at, /* Allocate space for a new ref_array_item and copy the objectname and flag to it */ static struct ref_array_item *new_ref_array_item(const char *refname, -const unsigned char *objectname, +const struct object_id *oid, int flag) { struct ref_array_item *ref; FLEX_ALLOC_STR(ref, refname, refname); - hashcpy(ref->objectname.hash, objectname); + oidcpy(&ref->objectname, oid); ref->flag = flag; return ref; @@ -1927,7 +1927,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid, * to do its job and the resulting list may yet to be pruned * by maxcount logic. */ - ref = new_ref_array_item(refname, oid->hash, flag); + ref = new_ref_array_item(refname, oid, flag); ref->commit = commit; REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1); @@ -2165,11 +2165,11 @@ void show_ref_array_item(struct ref_array_item *info, putchar('\n'); } -void pretty_print_ref(const char *name, const unsigned char *sha1, +void pretty_print_ref(const char *name, const struct object_id *oid, const struct ref_format *format) { struct ref_array_item *ref_item; - ref_item = new_ref_array_item(name, sha1, 0); + ref_item = new_ref_array_item(name, oid, 0); ref_item->kind = ref_kind_from_refname(name); show_ref_array_item(ref_item, format); free_array_item(ref_item); diff --git a/ref-filter.h b/ref-filter.h index 0d98342b3..68268f9eb 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -132,7 +132,7 @@ void setup_ref_filter_porcelain_msg(void); * Print a single ref, outside of any ref-filter. Note that the * name must be a fully qualified refname. */ -void pretty_print_ref(const char *name, const unsigned char *sha1, +void pretty_print_ref(const char *name, const struct object_id *oid, const struct ref_format *format); #endif /* REF_FILTER_H */ -- 2.14.3 (Apple Git-98)
[PATCH v14 3/4] ref-filter: factor ref_array pushing into its own function
From: Jeff King In preparation for callers constructing their own ref_array structs, let's move our own internal push operation into its own function. While we're at it, we can replace REALLOC_ARRAY() with ALLOC_GROW(), which should give the growth operation amortized linear complexity (as opposed to growing by one, which is potentially quadratic, though in-place realloc growth often makes this faster in practice). Signed-off-by: Jeff King Signed-off-by: Harald Nordgren --- ref-filter.c | 16 +--- ref-filter.h | 8 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index c1c3cc948..6e9328b27 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1840,6 +1840,18 @@ static struct ref_array_item *new_ref_array_item(const char *refname, return ref; } +struct ref_array_item *ref_array_push(struct ref_array *array, + const char *refname, + const struct object_id *oid) +{ + struct ref_array_item *ref = new_ref_array_item(refname, oid); + + ALLOC_GROW(array->items, array->nr + 1, array->alloc); + array->items[array->nr++] = ref; + + return ref; +} + static int ref_kind_from_refname(const char *refname) { unsigned int i; @@ -1930,13 +1942,11 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid, * to do its job and the resulting list may yet to be pruned * by maxcount logic. */ - ref = new_ref_array_item(refname, oid); + ref = ref_array_push(ref_cbdata->array, refname, oid); ref->commit = commit; ref->flag = flag; ref->kind = kind; - REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1); - ref_cbdata->array->items[ref_cbdata->array->nr++] = ref; return 0; } diff --git a/ref-filter.h b/ref-filter.h index 68268f9eb..76cf87cb6 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -135,4 +135,12 @@ void setup_ref_filter_porcelain_msg(void); void pretty_print_ref(const char *name, const struct object_id *oid, const struct ref_format *format); +/* + * Push a single ref onto the array; this can be used to construct your own + * ref_array without using filter_refs(). + */ +struct ref_array_item *ref_array_push(struct ref_array *array, + const char *refname, + const struct object_id *oid); + #endif /* REF_FILTER_H */ -- 2.14.3 (Apple Git-98)
[PATCH v14 2/4] ref-filter: make ref_array_item allocation more consistent
From: Jeff King We have a helper function to allocate ref_array_item structs, but it only takes a subset of the possible fields in the struct as initializers. We could have it accept an argument for _every_ field, but that becomes a pain for the fields which some callers don't want to set initially. Instead, let's be explicit that it takes only the minimum required to create the ref, and that callers should then fill in the rest themselves. Signed-off-by: Jeff King Signed-off-by: Harald Nordgren --- ref-filter.c | 18 +++--- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index ade97a848..c1c3cc948 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1824,15 +1824,18 @@ static const struct object_id *match_points_at(struct oid_array *points_at, return NULL; } -/* Allocate space for a new ref_array_item and copy the objectname and flag to it */ +/* + * Allocate space for a new ref_array_item and copy the name and oid to it. + * + * Callers can then fill in other struct members at their leisure. + */ static struct ref_array_item *new_ref_array_item(const char *refname, -const struct object_id *oid, -int flag) +const struct object_id *oid) { struct ref_array_item *ref; + FLEX_ALLOC_STR(ref, refname, refname); oidcpy(&ref->objectname, oid); - ref->flag = flag; return ref; } @@ -1927,12 +1930,13 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid, * to do its job and the resulting list may yet to be pruned * by maxcount logic. */ - ref = new_ref_array_item(refname, oid, flag); + ref = new_ref_array_item(refname, oid); ref->commit = commit; + ref->flag = flag; + ref->kind = kind; REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1); ref_cbdata->array->items[ref_cbdata->array->nr++] = ref; - ref->kind = kind; return 0; } @@ -2169,7 +2173,7 @@ void pretty_print_ref(const char *name, const struct object_id *oid, const struct ref_format *format) { struct ref_array_item *ref_item; - ref_item = new_ref_array_item(name, oid, 0); + ref_item = new_ref_array_item(name, oid); ref_item->kind = ref_kind_from_refname(name); show_ref_array_item(ref_item, format); free_array_item(ref_item); -- 2.14.3 (Apple Git-98)
[PATCH v14 4/4] ls-remote: create '--sort' option
Create a '--sort' option for ls-remote, based on the one from for-each-ref. This e.g. allows ref names to be sorted by version semantics, so that v1.2 is sorted before v1.10. Signed-off-by: Harald Nordgren --- Notes: Changes according to Junio Hamano's code review (2) Documentation/git-ls-remote.txt | 16 +++- builtin/ls-remote.c | 28 ++--- t/t5512-ls-remote.sh| 54 +++-- 3 files changed, 87 insertions(+), 11 deletions(-) diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt index 5f2628c8f..6ad1e34af 100644 --- a/Documentation/git-ls-remote.txt +++ b/Documentation/git-ls-remote.txt @@ -10,7 +10,7 @@ SYNOPSIS [verse] 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=] - [-q | --quiet] [--exit-code] [--get-url] + [-q | --quiet] [--exit-code] [--get-url] [--sort=] [--symref] [ [...]] DESCRIPTION @@ -60,6 +60,16 @@ OPTIONS upload-pack only shows the symref HEAD, so it will be the only one shown by ls-remote. +--sort=:: + Sort based on the key given. Prefix `-` to sort in descending order + of the value. Supports "version:refname" or "v:refname" (tag names + are treated as versions). The "version:refname" sort order can also + be affected by the "versionsort.suffix" configuration variable. + See linkgit:git-for-each-ref[1] for more sort options, but be aware + keys like `committerdate` that require access to the objects + themselves will not work for refs whose objects have not yet been + fetched from the remote, and will give a `missing object` error. + :: The "remote" repository to query. This parameter can be either a URL or the name of a remote (see the GIT URLS and @@ -90,6 +100,10 @@ EXAMPLES c5db5456ae3b0873fc659c19fafdde22313cc441refs/tags/v0.99.2 7ceca275d047c90c0c7d5afb13ab97efdf51bd6erefs/tags/v0.99.3 +SEE ALSO + +linkgit:git-check-ref-format[1]. + GIT --- Part of the linkgit:git[1] suite diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c index 540d56429..b26c53670 100644 --- a/builtin/ls-remote.c +++ b/builtin/ls-remote.c @@ -1,6 +1,7 @@ #include "builtin.h" #include "cache.h" #include "transport.h" +#include "ref-filter.h" #include "remote.h" static const char * const ls_remote_usage[] = { @@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) int show_symref_target = 0; const char *uploadpack = NULL; const char **pattern = NULL; + int i; struct remote *remote; struct transport *transport; const struct ref *ref; + struct ref_array ref_array; + static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting; struct option options[] = { OPT__QUIET(&quiet, N_("do not print remote URL")), @@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL), OPT_BOOL(0, "get-url", &get_url, N_("take url..insteadOf into account")), + OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"), +N_("field name to sort on"), &parse_opt_ref_sorting), OPT_SET_INT_F(0, "exit-code", &status, N_("exit with exit code 2 if no matching refs are found"), 2, PARSE_OPT_NOCOMPLETE), @@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) OPT_END() }; + memset(&ref_array, 0, sizeof(ref_array)); + argc = parse_options(argc, argv, prefix, options, ls_remote_usage, PARSE_OPT_STOP_AT_NON_OPTION); dest = argv[0]; @@ -90,6 +98,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) if (get_url) { printf("%s\n", *remote->url); + UNLEAK(sorting); return 0; } @@ -98,20 +107,33 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack); ref = transport_get_remote_refs(transport); - if (transport_disconnect(transport)) + if (transport_disconnect(transport)) { + UNLEAK(sorting); return 1; + } if (!dest && !quiet) fprintf(stderr, "From %s\n", *remote->url); for ( ; ref; ref = ref->next) { + struct ref_array_item *item; if (!check_ref_type(ref, flags)) continue; if (!tail_match(pattern, ref->name)) continue; + item = ref_array_push(&ref_array, ref->name, &ref->o
Re: [PATCH v13 4/4] ls-remote: create '--sort' option
On Mon, Apr 9, 2018 at 2:56 AM, Junio C Hamano wrote: > > With the update since v12, I think "because `ls-remote` deals only > with remotes," can be dropped entirely, and still convey what needs > to be told: "Be aware some keys that needs access to objects that > are not here won't work". > > Instead, it is probably a better idea to spend that half-line worth > of characters to describe in what way they do not work (do they try > to deref a NULL pointer and dump core? do they notice we need > missing objects and give an error?). Updated the docs again. The specific error we get on commiterdate is fatal: missing object f0d0fd3a5985d5e588da1e1d11c85fba0ae132f8 for refs/pull/10/head So no core dump. But a fatal error with a possibly confusing message.
Re: [RFC PATCH 4/7] dir: Directories should be checked for matching pathspecs too
Jeff King writes: > To be honest, I don't know. Most of dir.c predates me, and I've tried to > avoid looking at it too hard. But I had a vague recollection of it being > "best effort", and this bit from cf424f5fd89b reinforces that: > > However, read_directory does not actually check against our pathspec. > It uses a simplified version that may turn up false positives. As a > result, we need to check that any hits match our pathspec. At least the original design of the traversal was "try making use of pathspec during the traversal when we can cheaply filter out obvious non-hits and avoid recursing into an entire hierarchy---false negative is an absolute no-no, but forcing the consumer to post filter is OK". > ... But I think that anybody who looks at the output of > fill_directory() does need to be aware that they may get more entries > than they expected, and has to apply the pathspecs themselves. That matches with my understanding of how "fill" thing worked from early days.
Re: [PATCH] git-svn: avoid warning on undef readline()
Eric Wong writes: > Ævar Arnfjörð Bjarmason wrote: >> See https://public-inbox.org/git/86h8oobl36@phe.ftfl.ca/ for the >> original report. > > Thanks for taking a look at this. Also https://bugs.debian.org/894997 > >> --- a/perl/Git.pm >> +++ b/perl/Git.pm >> @@ -554,7 +554,7 @@ sub get_record { >> my ($fh, $rs) = @_; >> local $/ = $rs; >> my $rec = <$fh>; >> -chomp $rec if defined $rs; >> +chomp $rec if defined $rs and defined $rec; > > I'm struggling to understand the reason for the "defined $rs" > check. I think it was a braino on my part and meant to use: > > chomp $rec if defined $rec; That sounds quite plausible, so if there is no further discussion, let me queue a version that does s/rs/rec/ on that line myself (bypassing a pull from your repository at bogomips.org/). Thanks.
Re: [PATCH] t5404: relax overzealous test
Jeff King writes: > On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote: > >> In 0b294c0abf0 (make deleting a missing ref more quiet, 2008-07-08), we >> added a test to verify that deleting an already-deleted ref does not >> show an error. > > Amazing that it took this long to come up. > ... >> This patch chooses instead to look for the prefix "error:" at the >> beginning of the line, so that there can be no ambiguity that any catch >> was indeed a message generated by Git's `error_builtin()` function. > > Yep, this seems obviously correct. Hits in $ git grep 'grep ["'\'']*error' t shows that many checks for errors that are not anchored, but I do not think any of them are looking for the string in a pathname other than this instance, so it would be OK. Thanks, both. Will apply.
Re: [PATCH] Fix condition for redirecting stderr
Lucas Werkmeister wrote: > Since the --log-destination option was added in 0c591cacb ("daemon: add > --log-destination=(stderr|syslog|none)", 2018-02-04) with the explicit > goal of allowing logging to stderr when running in inetd mode, we should > not always redirect stderr to /dev/null in inetd mode, but rather only > when stderr is not being used for logging. Perhaps 's/^F/daemon: f/' on the subject? (Junio may well already have done so while queueing locally.) The patch itself looks reasonable (to my relatively untrained eyes). -- Todd ~~ Hardware: the parts of a computer that can be kicked. -- Jeff Pesis
Re: [PATCH v12 4/4] ls-remote: create '--sort' option
On Sun, Apr 8, 2018 at 6:16 PM, Junio C Hamano wrote: > Harald Nordgren writes: >> +--sort=:: >> + Sort based on the key given. [...] > > This is a tangent but I suspect that the description of --sort in > git-for-each-ref documentation is wrong on the use of multiple > options, or I am misreading parse_opt_ref_sorting(), which I think > appends later keys to the end of the list, and compare_refs() which > I think yields results when an earlier key in the last decides two > are not equal and ignores the later keys. The commit that added the > description was c0f6dc9b ("git-for-each-ref.txt: minor > improvements", 2008-06-05), and I think even back then the code in > builtin-for-each-ref.c appended later keys at the end, and it seems > nobody complained, so it is possible I am not reading the code right > before fully adjusting to timezone change ;-) I just re-read parse_opt_ref_sorting(), and I'm pretty sure that it is _prepending_ keys to the list, despite the confusingly named variable "sorting_tail", thus it agrees with the documentation that --sort= specified last becomes primary sort key. For me, at least, that behavior (last --sort= becomes primary key) feels counterintuitive, but it's too late to change it now.
Re: [PATCH] t/helper: 'test-chmtime (--get|-g)' to print only the mtime
Paul-Sebastian Ungureanu writes: > Compared to 'test-chmtime -v +0 file' which prints the mtime and > and the file name, 'test-chmtime --get file' displays only the mtime. > If it is used in combination with (+|=|=+|=-|-)seconds, it changes > and prints the new value. > > test-chmtime -v +0 file | sed 's/[^0-9].*$//' > > is now equivalent to: > > test-chmtime --get file > > Signed-off-by: Paul-Sebastian Ungureanu > --- > t/helper/test-chmtime.c | 44 +++- > t/t2022-checkout-paths.sh| 4 +-- > t/t3404-rebase-interactive.sh| 2 +- > t/t3510-cherry-pick-sequence.sh | 4 +-- > t/t4200-rerere.sh| 8 ++--- > t/t5000-tar-tree.sh | 2 +- > t/t6022-merge-rename.sh | 25 +++- > t/t6501-freshen-objects.sh | 6 ++-- > t/t7508-status.sh| 4 +-- > t/t7701-repack-unpack-unreachable.sh | 6 ++-- > 10 files changed, 63 insertions(+), 42 deletions(-) Thanks both for suggesting and implementing an obvious improvement and updating many places that are helped by it. Will queue. .
Re: [PATCH] t/helper: 'test-chmtime (--get|-g)' to print only the mtime
Paul-Sebastian Ungureanu writes: > - find .git/rr-cache/ -type f | xargs test-chmtime -172800 && > + test-chmtime -172800 $(find .git/rr-cache/ -type f) && You've sneaked this kind of rewrite in, as if you are testing to see how careful the reviewers are ;-). We often use "find piped to xargs" pattern to avoid unbounded number of paths from appearing on a command line, busting platform limits. In the case of these test scripts, it does not matter very much, but such a "we can save a process and a pipe this way" optimization is not within the scope of "chmtime +v often is piped to sed only to strip path---let's give it a way to just grab the timestamp" topic. Not a very welcome change. Having said that, I do not want this to be rerolled if this unrelated "removal of find-piped-to-xargs pattern" is the only niggle in the patch, as I've already checked if these conversions are safe (they are---we are not dealing with hundreds of stuff in .git/objects/ or .git/rr-cache/). There were conflicts caused by this patch and nd/combined-test-helper (which makes the test-chmtime standalone binary as a subcommand of test-tool); I think I resolved them correctly, but please double check when I update the public repositories in several hours. Thanks.
Re: [PATCH/RFC 2/5] git.c: implement --list-cmds=all and use it in git-completion.bash
On Mon, Mar 26, 2018 at 12:55 PM, Nguyễn Thái Ngọc Duy wrote: > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > diff --git a/help.c b/help.c > @@ -228,6 +228,21 @@ void list_common_cmds_help(void) > +void list_all_cmds(void) > +{ > + struct cmdnames main_cmds, other_cmds; > + int i; > + > + memset(&main_cmds, 0, sizeof(main_cmds)); > + memset(&other_cmds, 0, sizeof(other_cmds)); > + load_command_list("git-", &main_cmds, &other_cmds); > + > + for (i = 0; i < main_cmds.cnt; i++) > + puts(main_cmds.names[i]->name); > + for (i = 0; i < other_cmds.cnt; i++) > + puts(other_cmds.names[i]->name); clean_cmdnames(&main_cmds); clean_cmdnames(&other_cmds); > +}
Re: [PATCH 3/3] ref-filter: factor ref_array pushing into its own function
On Mon, Apr 09, 2018 at 08:18:35AM +0900, Junio C Hamano wrote: > Jeff King writes: > > > In preparation for callers constructing their own ref_array > > structs, let's move our own internal push operation into its > > own function. > > > > While we're at it, we can replace REALLOC_ARRAY() with > > ALLOC_GROW(), which should give the growth operation > > amortized linear complexity (as opposed to growing by one, > > which is potentially quadratic, though in-place realloc > > growth often makes this faster in practice). > > Sorry, but I do not quite get the last part this paragraph. Up to > but not including ", though in-place..." I would understand as: > > - With ALLOC_GROW(), we are explicit in that we are amortizing >the allocation cost by growing in exponential batches. > > - With REALLOC_ARRAY(), we are telling the underlying >realloc(3) that it is OK to pretend that we grow by one, but >if that permission is literally followed, it could end up >quadratic. > > So if you have really a bad realloc(3) implementation, switching > to use ALLOC_GROW() is an improvement. Yes, that's the gist of what I was saying. Though it is not even necessarily "a bad realloc". At some point you may hit heap segmentation and need to copy (though I guess if that happens repeatedly then maybe your realloc really _is_ bad ;) ). > But after that I am not sure what you are getting at. Do you mean > "In practice, a well-done realloc(3) does the amortizing internally > anyway, which makes it faster than the grow-by-one quadratic, so > this change may not make that much of a difference"? Or do you mean > "In practice, a well-done realloc(3) internally amortizes better > than we do manually, so this change may hurt performance"? The first. I couldn't measure any speedup on glibc, which makes me think it's mostly doing in-place expansion. > In either case, I think this splitting into a helper is obviously a > good move, and using ALLOC_GROW() is conceptually cleaner, as we use > it everywhere and tend to avoid realloc(3) just to add one item. > > Other than that small confusion (not even a nit), three patches were > pleasant read. Thanks. Thanks. Please feel free to expand or clarify the commit message if that helps. -Peff
Re: [PATCH 1/1] perl: fix installing modules from contrib
Ævar Arnfjörð Bjarmason writes: >> >> +perllibdir: >> +@echo $(perllibdir_SQ) This use of _SQ variant is fishy, isn't it? Judging from the output of $ git grep _SQ Makefile e.g. Makefile: $(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' Makefile: $(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' Makefile: $(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' I'd expect that any _SQ variant must be referenced inside a single quote pair. In fact, that is why a single quote (and nothing else) in the base variable is replaced with the magic "'\''" sequence, first stepping out of the current sq context, append a single sq (escaped with a backslash from the shell), and then stepping back into another sq context. I think nobody saw breakage only because they do not have two consecutive SPs (or any single quote) in their path to $perllibdir. If we depend on such limitation, there is no point using _SQ variant, but we already have _SQ variant, let's use it correctly.
Re: [PATCH/RFC 3/5] generate-cmdlist.sh: keep all information in common-cmds.h
On Mon, Mar 26, 2018 at 12:55 PM, Nguyễn Thái Ngọc Duy wrote: > common-cmds.h is used to extract the list of common commands (by > group) and a one-line summary of each command. Some information is > dropped, for example command category or summary of other commands. > Update generate-cmdlist.sh to keep all the information. The extra info > will be used shortly. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh > @@ -2,9 +2,10 @@ > struct cmdname_help { > - char name[16]; > + char name[32]; > char help[80]; > - unsigned char group; > + unsigned int category; > + unsigned int group; > }; > @@ -23,27 +24,50 @@ sed -n ' > +echo "#define GROUP_NONE 0xff /* no common group */" > +echo "#define GROUP_ 0xff /* no common group */" Meh, this "GROUP_" alias of "GROUP_NONE" isn't so nice. > n=0 > -substnum= > while read grp > do > - echo "^git-..*[ ]$grp" > - substnum="$substnum${substnum:+;}s/[]$grp/$n/" > + echo "#define GROUP_$grp $n" > n=$(($n+1)) > -done <"$grps" >"$match" > +done <"$grps" This patch drops all use of the file $match. Earlier in this script, not seen in the context, are a couple references to $match which ought to be adjusted to take its retirement into account: match=match$$.tmp trap "rm -f '$grps' '$match'" 0 1 2 3 15 However, I'm concerned that this change may be going in the wrong direction. A line in "### command list" section looks like this: command-name category [deprecated] [common] Although we don't currently have any commands marked with tag "deprecated", we very well may have some day. More generally, new optional or required tags may be added in the future. As such, the line format is relatively free-form. Current clients don't even care in what order the tags appears (following 'category') nor how many tags there are. The new code added by this patch, however, is far less flexible and accommodating since it assumes hard-coded columns for the tags (and doesn't even take 'deprecated' into account). The point of the $match file was to be able to extract only lines which mentioned one of the "common groups", and the point of the 'substnum' transformation was to transform the group name into a group number -- both of these operations were done without caring about the exact column the "common group" tag occupied. Obviously, one option for addressing this concern would be to change the definition to make the tag columns fixed and non-optional, which would allow the simpler implementation used by this patch. Doing so may require fixing other consumers of command-list.txt (though, I'm pretty sure existing consumers wouldn't be bothered). (Perl would be an obvious good choice for retaining the current relatively free-form line definition without having to jump through hoops in the shell. Unfortunately, though, a Perl dependency in the build system can be problematic[1].) [1]: https://public-inbox.org/git/1440365469-9928-1-git-send-email-sunsh...@sunshineco.com/ > -printf 'static struct cmdname_help common_cmds[] = {\n' > -grep -f "$match" "$1" | > +echo '/*' > +printf 'static const char *cmd_categories[] = {\n' > +grep '^git-' "$1" | This "grep '^git-'" (and those below) misses some commands, such as "gitk" and "gitweb". Is that intentional? If not, then you'll probably need to grab lines following "### command list", as is done earlier in the script. Same comment for the other couple grep's later in the patch. > +awk '{print $2;}' | At one time, Junio expressed concerns[2] about having an 'awk' dependency in the build system (in fact, with regards to this same generation process). Whether he still has such concerns is unknown, but it should be easy enough to avoid it here (and below). [2]: https://public-inbox.org/git/20150519004356.GA12854@flurp.local/ > +sort | > +uniq | > +while read category; do > + printf '\t\"'$category'\",\n' > +done > +printf '\tNULL\n};\n\n' > +echo '*/' > diff --git a/help.c b/help.c > @@ -190,6 +190,28 @@ void list_commands(unsigned int colopts, > +static void extract_common_cmds(struct cmdname_help **p_common_cmds, > + int *p_nr) > +{ > + int i, nr = 0; > + struct cmdname_help *common_cmds; > + > + ALLOC_ARRAY(common_cmds, ARRAY_SIZE(command_list)); > + > + for (i = 0; i < ARRAY_SIZE(command_list); i++) { > + const struct cmdname_help *cmd = command_list + i; > + > + if (cmd->category != CAT_mainporcelain || > + cmd->group == GROUP_NONE) > + continue; Is the CAT_mainporcelain condition necessary? Before this patch, the command list would contain only commands with an associated group, so it seems that you could get by just with the GROUP_NONE condition. > + > + common_cmds[nr++] = *cmd; > + } > + > + *p_common_cmds = common_cmds; > + *p_nr
Re: [PATCH/RFC 5/5] help: add "-a --verbose" to list all commands with synopsis
On Mon, Mar 26, 2018 at 12:55 PM, Nguyễn Thái Ngọc Duy wrote: > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > diff --git a/help.c b/help.c > @@ -282,6 +282,67 @@ void list_porcelain_cmds(void) > +static const char *get_category_name(unsigned int category) > +{ > + switch (category) { > + case CAT_ancillaryinterrogators: return _("Ancillary interrogators"); > + case CAT_ancillarymanipulators: return _("Ancillary manipulators"); > + case CAT_foreignscminterface: return _("Foreign SCM interface"); > + case CAT_mainporcelain: return _("Main porcelain"); > + case CAT_plumbinginterrogators: return _("Plumbing interrogators"); > + case CAT_plumbingmanipulators: return _("Plumbing interrogators"); s/interrogators"/manipulators"/ > + case CAT_purehelpers: return _("Pure helpers"); > + case CAT_synchelpers: return _("Sync helpers"); > + case CAT_synchingrepositories: return _("Synching repositories"); > + default: > + die("BUG: unknown command category %u", category); > + }
Re: [PATCH/RFC 3/5] generate-cmdlist.sh: keep all information in common-cmds.h
On Mon, Apr 9, 2018 at 12:59 AM, Eric Sunshine wrote: > However, I'm concerned that this change may be going in the wrong > direction. A line in "### command list" section looks like this: > > command-name category [deprecated] [common] > > Although we don't currently have any commands marked with tag > "deprecated", we very well may have some day. More generally, new > optional or required tags may be added in the future. As such, the > line format is relatively free-form. Current clients don't even care > in what order the tags appears (following 'category') nor how many > tags there are. The new code added by this patch, however, is far less > flexible and accommodating since it assumes hard-coded columns for the > tags (and doesn't even take 'deprecated' into account). > > The point of the $match file was to be able to extract only lines > which mentioned one of the "common groups", and the point of the > 'substnum' transformation was to transform the group name into a group > number -- both of these operations were done without caring about the > exact column the "common group" tag occupied. > > Obviously, one option for addressing this concern would be to change > the definition to make the tag columns fixed and non-optional, which > would allow the simpler implementation used by this patch. Doing so > may require fixing other consumers of command-list.txt (though, I'm > pretty sure existing consumers wouldn't be bothered). I should follow up by saying that, although the current relatively free-form line definition is nice due to its flexibility, considering how infrequently command-list.txt is edited and how much more complex the script is to support that flexibility, changing to a definition in which tags are required and at fixed columns seems like the pragmatic thing to do.
Re: [PATCH/RFC 0/5] Keep all info in command-list.txt in git binary
On Mon, Mar 26, 2018 at 12:55 PM, Nguyễn Thái Ngọc Duy wrote: > This is pretty rough but I'd like to see how people feel about this > first. > > I notice we have two places for command classification. One in > command-list.txt, one in __git_list_porcelain_commands() in > git-completion.bash. People who are following nd/parseopt-completion > probably know that I'm try to reduce duplication in this script as > much as possible, this is another step towards that. > > By keeping all information of command-list.txt in git binary, we could > provide the porcelain list to git-completion.bash via "git > --list-cmds=porcelain", so we don't neeed a separate command > classification in git-completion.bash anymore. I like the direction this series is taking. > Because we have all command synopsis as a side effect, we could > now support "git help -a --verbose" which prints something like "git > help", a command name and a description, but we could do it for _all_ > recognized commands. This could help people look for a command even if > we don't provide "git appropos". Nice idea, and you practically get this for free (aside from the the obvious new code) since generate-cmdlist.sh already plucks the summary for each command directly from Documentation/git-*.txt.