Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
On Fri, Oct 7, 2016 at 10:55 PM, Jakub Narębski wrote: > W dniu 07.10.2016 o 16:19, Johannes Schindelin pisze: >> On Fri, 7 Oct 2016, Jakub Narębski wrote: > >>> Note that we would have to teach git completion about new syntax; >>> or new configuration variable if we go that route. >> >> Why would we? Git's completion does not expand aliases, it only completes >> the aliases' names, not their values. > > Because Git completion finds out which _options_ and _arguments_ > to complete for an alias based on its expansion. > > Yes, this is nice bit of trickery... It's c63437c (bash: improve aliased command recognition - 2010-02-23) isn't it? This may favor j6t's approach [1] because retrieving alias attributes is much easier. [1] https://public-inbox.org/git/20161006190720.4ecf3ptl6mszt...@sigill.intra.peff.net/T/#mb1d7b8f31d595b05105b8ea2137756761e13dbf4 -- Duy
[PATCH] remote.c: free previous results when looking for a ref match
Signed-off-by: Stefan Beller --- remote.c | 4 1 file changed, 4 insertions(+) diff --git a/remote.c b/remote.c index ad6c542..5f9afb4 100644 --- a/remote.c +++ b/remote.c @@ -833,6 +833,8 @@ static int match_name_with_pattern(const char *key, const char *name, strbuf_add(&sb, value, vstar - value); strbuf_add(&sb, name + klen, namelen - klen - ksuffixlen); strbuf_addstr(&sb, vstar + 1); + if (*result) + free(*result); *result = strbuf_detach(&sb, NULL); } return ret; @@ -1262,6 +1264,8 @@ static char *get_ref_match(const struct refspec *rs, int rs_nr, const struct ref */ if (!send_mirror && !starts_with(ref->name, "refs/heads/")) return NULL; + if (name) + free(name); name = xstrdup(ref->name); } if (ret_pat) -- 2.10.1.353.g1629400
[PATCH v4 4/4] mergetool: honor -O
Teach mergetool to pass "-O" down to `git diff` when specified on the command-line. Helped-by: Johannes Sixt Signed-off-by: David Aguilar --- Since v3: I missed one last piped invocation of "git mergetool" in the tests, which has been fixed. Documentation/git-mergetool.txt | 10 ++ git-mergetool.sh| 9 +++-- t/t7610-mergetool.sh| 27 +++ 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt index c4c1a9b..3622d66 100644 --- a/Documentation/git-mergetool.txt +++ b/Documentation/git-mergetool.txt @@ -79,10 +79,12 @@ success of the resolution after the custom tool has exited. Prompt before each invocation of the merge resolution program to give the user a chance to skip the path. -DIFF ORDER FILES - -`git mergetool` honors the `diff.orderFile` configuration variable -used by `git diff`. See linkgit:git-config[1] for more details. +-O:: + Process files in the order specified in the + , which has one shell glob pattern per line. + This overrides the `diff.orderFile` configuration variable + (see linkgit:git-config[1]). To cancel `diff.orderFile`, + use `-O/dev/null`. TEMPORARY FILES --- diff --git a/git-mergetool.sh b/git-mergetool.sh index 65696d8..e52b4e4 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -9,7 +9,7 @@ # at the discretion of Junio C Hamano. # -USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...' +USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-O] [file to merge] ...' SUBDIRECTORY_OK=Yes NONGIT_OK=Yes OPTIONS_SPEC= @@ -390,6 +390,7 @@ print_noop_and_exit () { main () { prompt=$(git config --bool mergetool.prompt) guessed_merge_tool=false + orderfile= while test $# != 0 do @@ -419,6 +420,9 @@ main () { --prompt) prompt=true ;; + -O*) + orderfile="$1" + ;; --) shift break @@ -460,7 +464,8 @@ main () { fi files=$(git -c core.quotePath=false \ - diff --name-only --diff-filter=U -- "$@") + diff --name-only --diff-filter=U \ + ${orderfile:+"$orderfile"} -- "$@") cd_to_toplevel diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh index 38c1e4d..6d9f215 100755 --- a/t/t7610-mergetool.sh +++ b/t/t7610-mergetool.sh @@ -638,5 +638,32 @@ test_expect_success 'diff.orderFile configuration is honored' ' test_cmp expect actual && git reset --hard >/dev/null ' +test_expect_success 'mergetool -Oorder-file is honored' ' + test_config diff.orderFile order-file && + test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" && + test_config mergetool.myecho.trustExitCode true && + test_must_fail git merge order-file-side1 && + cat >expect <<-\EOF && + Merging: + a + b + EOF + git mergetool -O/dev/null --no-prompt --tool myecho >output && + git grep --no-index -h -A2 Merging: output >actual && + test_cmp expect actual && + git reset --hard >/dev/null 2>&1 && + + git config --unset diff.orderFile && + test_must_fail git merge order-file-side1 && + cat >expect <<-\EOF && + Merging: + b + a + EOF + git mergetool -Oorder-file --no-prompt --tool myecho >output && + git grep --no-index -h -A2 Merging: output >actual && + test_cmp expect actual && + git reset --hard >/dev/null 2>&1 +' test_done -- 2.10.1.386.g8ee99a0
[PATCH v3 2/4] mergetool: move main program flow into a main() function
Make it easier to follow the program's flow by isolating all logic into functions. Isolate the main execution code path into a single unit instead of having prompt_after_failed_merge() interrupt it partyway through. The use of a main() function is borrowing a convention from C, Python, Perl, and many other languages. This helps readers more familiar with other languages understand the purpose of each function when diving into the codebase with fresh eyes. Signed-off-by: David Aguilar --- Unchanged since v2; included for completeness. git-mergetool.sh | 180 --- 1 file changed, 93 insertions(+), 87 deletions(-) diff --git a/git-mergetool.sh b/git-mergetool.sh index 300ce7f..b2cd0a4 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -366,51 +366,6 @@ merge_file () { return 0 } -prompt=$(git config --bool mergetool.prompt) -guessed_merge_tool=false - -while test $# != 0 -do - case "$1" in - --tool-help=*) - TOOL_MODE=${1#--tool-help=} - show_tool_help - ;; - --tool-help) - show_tool_help - ;; - -t|--tool*) - case "$#,$1" in - *,*=*) - merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)') - ;; - 1,*) - usage ;; - *) - merge_tool="$2" - shift ;; - esac - ;; - -y|--no-prompt) - prompt=false - ;; - --prompt) - prompt=true - ;; - --) - shift - break - ;; - -*) - usage - ;; - *) - break - ;; - esac - shift -done - prompt_after_failed_merge () { while true do @@ -427,57 +382,108 @@ prompt_after_failed_merge () { done } -git_dir_init -require_work_tree +main () { + prompt=$(git config --bool mergetool.prompt) + guessed_merge_tool=false + + while test $# != 0 + do + case "$1" in + --tool-help=*) + TOOL_MODE=${1#--tool-help=} + show_tool_help + ;; + --tool-help) + show_tool_help + ;; + -t|--tool*) + case "$#,$1" in + *,*=*) + merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)') + ;; + 1,*) + usage ;; + *) + merge_tool="$2" + shift ;; + esac + ;; + -y|--no-prompt) + prompt=false + ;; + --prompt) + prompt=true + ;; + --) + shift + break + ;; + -*) + usage + ;; + *) + break + ;; + esac + shift + done + + git_dir_init + require_work_tree -if test -z "$merge_tool" -then - # Check if a merge tool has been configured - merge_tool=$(get_configured_merge_tool) - # Try to guess an appropriate merge tool if no tool has been set. if test -z "$merge_tool" then - merge_tool=$(guess_merge_tool) || exit - guessed_merge_tool=true + # Check if a merge tool has been configured + merge_tool=$(get_configured_merge_tool) + # Try to guess an appropriate merge tool if no tool has been set. + if test -z "$merge_tool" + then + merge_tool=$(guess_merge_tool) || exit + guessed_merge_tool=true + fi fi -fi -merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)" -merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)" - -files= + merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)" + merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)" -if test $# -eq 0 -then - cd_to_toplevel + files= - if test -e "$GIT_DIR/MERGE_RR" + if test $# -eq 0 then - files=$(git rerere remaining) + cd_to_toplevel + + if test -e "$GIT_DIR/MERGE_RR" + then + files=$(git rerere remaining) + else + files=$(git ls-files -u | +
[PATCH v3 3/4] mergetool: honor diff.orderFile
Teach mergetool to get the list of files to edit via `diff` so that we gain support for diff.orderFile. Suggested-by: Luis Gutierrez Helped-by: Johannes Sixt Signed-off-by: David Aguilar --- Changes since v2: The tests no longer rely on "grep -A" and instead use "git grep" for portability. The mergetool output in the test is redirected to a file so that we can catch its exit code. Documentation/git-mergetool.txt | 5 + git-mergetool.sh| 30 +++--- t/t7610-mergetool.sh| 33 + 3 files changed, 53 insertions(+), 15 deletions(-) diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt index e846c2e..c4c1a9b 100644 --- a/Documentation/git-mergetool.txt +++ b/Documentation/git-mergetool.txt @@ -79,6 +79,11 @@ success of the resolution after the custom tool has exited. Prompt before each invocation of the merge resolution program to give the user a chance to skip the path. +DIFF ORDER FILES + +`git mergetool` honors the `diff.orderFile` configuration variable +used by `git diff`. See linkgit:git-config[1] for more details. + TEMPORARY FILES --- `git mergetool` creates `*.orig` backup files while resolving merges. diff --git a/git-mergetool.sh b/git-mergetool.sh index b2cd0a4..65696d8 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -382,6 +382,11 @@ prompt_after_failed_merge () { done } +print_noop_and_exit () { + echo "No files need merging" + exit 0 +} + main () { prompt=$(git config --bool mergetool.prompt) guessed_merge_tool=false @@ -445,28 +450,23 @@ main () { merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)" merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)" - files= - - if test $# -eq 0 + if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR" then - cd_to_toplevel - - if test -e "$GIT_DIR/MERGE_RR" + set -- $(git rerere remaining) + if test $# -eq 0 then - files=$(git rerere remaining) - else - files=$(git ls-files -u | - sed -e 's/^[^ ]* //' | sort -u) + print_noop_and_exit fi - else - files=$(git ls-files -u -- "$@" | - sed -e 's/^[^ ]* //' | sort -u) fi + files=$(git -c core.quotePath=false \ + diff --name-only --diff-filter=U -- "$@") + + cd_to_toplevel + if test -z "$files" then - echo "No files need merging" - exit 0 + print_noop_and_exit fi printf "Merging:\n" diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh index 7217f39..38c1e4d 100755 --- a/t/t7610-mergetool.sh +++ b/t/t7610-mergetool.sh @@ -606,4 +606,37 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT git reset --hard master >/dev/null 2>&1 ' +test_expect_success 'diff.orderFile configuration is honored' ' + test_config diff.orderFile order-file && + test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" && + test_config mergetool.myecho.trustExitCode true && + echo b >order-file && + echo a >>order-file && + git checkout -b order-file-start master && + echo start >a && + echo start >b && + git add a b && + git commit -m start && + git checkout -b order-file-side1 order-file-start && + echo side1 >a && + echo side1 >b && + git add a b && + git commit -m side1 && + git checkout -b order-file-side2 order-file-start && + echo side2 >a && + echo side2 >b && + git add a b && + git commit -m side2 && + test_must_fail git merge order-file-side1 && + cat >expect <<-\EOF && + Merging: + b + a + EOF + git mergetool --no-prompt --tool myecho >output && + git grep --no-index -h -A2 Merging: output >actual && + test_cmp expect actual && + git reset --hard >/dev/null +' + test_done -- 2.10.1.386.g8ee99a0
[PATCH v3 4/4] mergetool: honor -O
Teach mergetool to pass "-O" down to `git diff` when specified on the command-line. Helped-by: Johannes Sixt Signed-off-by: David Aguilar --- Changes since v2: The tests no longer rely on "grep -A" and instead use "git grep" for portability. The mergetool output in the test is redirected to a file so that we can catch its exit code. The $orderfile variable is now passed using ${xxx:+"$xxx"} to avoid conditional logic. Documentation/git-mergetool.txt | 10 ++ git-mergetool.sh| 9 +++-- t/t7610-mergetool.sh| 27 +++ 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt index c4c1a9b..3622d66 100644 --- a/Documentation/git-mergetool.txt +++ b/Documentation/git-mergetool.txt @@ -79,10 +79,12 @@ success of the resolution after the custom tool has exited. Prompt before each invocation of the merge resolution program to give the user a chance to skip the path. -DIFF ORDER FILES - -`git mergetool` honors the `diff.orderFile` configuration variable -used by `git diff`. See linkgit:git-config[1] for more details. +-O:: + Process files in the order specified in the + , which has one shell glob pattern per line. + This overrides the `diff.orderFile` configuration variable + (see linkgit:git-config[1]). To cancel `diff.orderFile`, + use `-O/dev/null`. TEMPORARY FILES --- diff --git a/git-mergetool.sh b/git-mergetool.sh index 65696d8..e52b4e4 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -9,7 +9,7 @@ # at the discretion of Junio C Hamano. # -USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...' +USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-O] [file to merge] ...' SUBDIRECTORY_OK=Yes NONGIT_OK=Yes OPTIONS_SPEC= @@ -390,6 +390,7 @@ print_noop_and_exit () { main () { prompt=$(git config --bool mergetool.prompt) guessed_merge_tool=false + orderfile= while test $# != 0 do @@ -419,6 +420,9 @@ main () { --prompt) prompt=true ;; + -O*) + orderfile="$1" + ;; --) shift break @@ -460,7 +464,8 @@ main () { fi files=$(git -c core.quotePath=false \ - diff --name-only --diff-filter=U -- "$@") + diff --name-only --diff-filter=U \ + ${orderfile:+"$orderfile"} -- "$@") cd_to_toplevel diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh index 38c1e4d..5cfad76 100755 --- a/t/t7610-mergetool.sh +++ b/t/t7610-mergetool.sh @@ -638,5 +638,32 @@ test_expect_success 'diff.orderFile configuration is honored' ' test_cmp expect actual && git reset --hard >/dev/null ' +test_expect_success 'mergetool -Oorder-file is honored' ' + test_config diff.orderFile order-file && + test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" && + test_config mergetool.myecho.trustExitCode true && + test_must_fail git merge order-file-side1 && + cat >expect <<-\EOF && + Merging: + a + b + EOF + git mergetool -O/dev/null --no-prompt --tool myecho | + grep -A 2 Merging: >actual && + test_cmp expect actual && + git reset --hard >/dev/null 2>&1 && + + git config --unset diff.orderFile && + test_must_fail git merge order-file-side1 && + cat >expect <<-\EOF && + Merging: + b + a + EOF + git mergetool -Oorder-file --no-prompt --tool myecho >output && + git grep --no-index -h -A2 Merging: output >actual && + test_cmp expect actual && + git reset --hard >/dev/null 2>&1 +' test_done -- 2.10.1.386.g8ee99a0
[PATCH v3 1/4] mergetool: add copyright
Signed-off-by: David Aguilar --- Unchanged since v1; included for completeness. git-mergetool.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/git-mergetool.sh b/git-mergetool.sh index bf86270..300ce7f 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -3,6 +3,7 @@ # This program resolves merge conflicts in git # # Copyright (c) 2006 Theodore Y. Ts'o +# Copyright (c) 2009-2016 David Aguilar # # This file is licensed under the GPL v2, or a later version # at the discretion of Junio C Hamano. -- 2.10.1.386.g8ee99a0
[PATCH v4 6/7] t/t7030-verify-tag: Add --format specifier tests
From: Santiago Torres Verify-tag now provides --format specifiers to inspect and ensure the contents of the tag are proper. We add two tests to ensure this functionality works as expected: the return value should indicate if verification passed, and the format specifiers must be respected. Signed-off-by: Santiago Torres --- t/t7030-verify-tag.sh | 16 1 file changed, 16 insertions(+) diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh index 07079a4..ce37fd9 100755 --- a/t/t7030-verify-tag.sh +++ b/t/t7030-verify-tag.sh @@ -125,4 +125,20 @@ test_expect_success GPG 'verify multiple tags' ' test_cmp expect.stderr actual.stderr ' +test_expect_success 'verifying tag with --format' ' + cat >expect <<-\EOF && + tagname : fourth-signed + EOF + git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual && + test_cmp expect actual +' + +test_expect_success 'verifying a forged tag with --format fail and format accordingly' ' + cat >expect <<-\EOF && + tagname : 7th forged-signed + EOF + test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged && + test_cmp expect actual-forged +' + test_done -- 2.10.0
[PATCH v4 5/7] builtin/tag: add --format argument for tag -v
From: Lukas Puehringer Adding --format to git tag -v mutes the default output of the GPG verification and instead prints the formatted tag object. This allows callers to cross-check the tagname from refs/tags with the tagname from the tag object header upon GPG verification. The callback function for for_each_tag_name() didn't allow callers to pass custom data to their callback functions. Add a new opaque pointer to each_tag_name_fn's parameter to allow this. Signed-off-by: Lukas Puehringer --- Documentation/git-tag.txt | 2 +- builtin/tag.c | 34 +++--- builtin/verify-tag.c | 2 +- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt index 7ecca8e..3bb5e3c 100644 --- a/Documentation/git-tag.txt +++ b/Documentation/git-tag.txt @@ -15,7 +15,7 @@ SYNOPSIS 'git tag' [-n[]] -l [--contains ] [--points-at ] [--column[=] | --no-column] [--create-reflog] [--sort=] [--format=] [--[no-]merged []] [...] -'git tag' -v ... +'git tag' -v [--format=] ... DESCRIPTION --- diff --git a/builtin/tag.c b/builtin/tag.c index 14f3b48..7730fd0 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -24,7 +24,7 @@ static const char * const git_tag_usage[] = { N_("git tag -d ..."), N_("git tag -l [-n[]] [--contains ] [--points-at ]" "\n\t\t[--format=] [--[no-]merged []] [...]"), - N_("git tag -v ..."), + N_("git tag -v [--format=] ..."), NULL }; @@ -66,15 +66,17 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con } typedef int (*each_tag_name_fn)(const char *name, const char *ref, - const unsigned char *sha1); + const unsigned char *sha1, void *cb_data); -static int for_each_tag_name(const char **argv, each_tag_name_fn fn) +static int for_each_tag_name(const char **argv, each_tag_name_fn fn, + void *cb_data) { const char **p; char ref[PATH_MAX]; int had_error = 0; unsigned char sha1[20]; + for (p = argv; *p; p++) { if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p) >= sizeof(ref)) { @@ -87,14 +89,14 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn) had_error = 1; continue; } - if (fn(*p, ref, sha1)) + if (fn(*p, ref, sha1, cb_data)) had_error = 1; } return had_error; } static int delete_tag(const char *name, const char *ref, - const unsigned char *sha1) + const unsigned char *sha1, void *cb_data) { if (delete_ref(ref, sha1, 0)) return 1; @@ -103,9 +105,16 @@ static int delete_tag(const char *name, const char *ref, } static int verify_tag(const char *name, const char *ref, - const unsigned char *sha1) + const unsigned char *sha1, void *cb_data) { - return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE); + int flags; + char *fmt_pretty = cb_data; + flags = GPG_VERIFY_VERBOSE; + + if (fmt_pretty) + flags = GPG_VERIFY_QUIET; + + return verify_and_format_tag(sha1, name, fmt_pretty, flags); } static int do_sign(struct strbuf *buffer) @@ -334,7 +343,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) struct strbuf err = STRBUF_INIT; struct ref_filter filter; static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting; - const char *format = NULL; + char *format = NULL; struct option options[] = { OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'), { OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"), @@ -424,9 +433,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix) if (filter.merge_commit) die(_("--merged and --no-merged option are only allowed with -l")); if (cmdmode == 'd') - return for_each_tag_name(argv, delete_tag); - if (cmdmode == 'v') - return for_each_tag_name(argv, verify_tag); + return for_each_tag_name(argv, delete_tag, NULL); + if (cmdmode == 'v') { + if (format) + verify_ref_format(format); + return for_each_tag_name(argv, verify_tag, format); + } if (msg.given || msgfile) { if (msg.given && msgfile) -- 2.10.0
[PATCH v4 7/7] t/t7004-tag: Add --format specifier tests
From: Santiago Torres tag -v now supports --format specifiers to inspect the contents of a tag upon verification. Add two tests to ensure this behavior is respected in future changes. Signed-off-by: Santiago Torres --- t/t7004-tag.sh | 16 1 file changed, 16 insertions(+) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 8b0f71a..633b089 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -847,6 +847,22 @@ test_expect_success GPG 'verifying a forged tag should fail' ' test_must_fail git tag -v forged-tag ' +test_expect_success 'verifying a proper tag with --format pass and format accordingly' ' + cat >expect <<-\EOF && + tagname : signed-tag + EOF + git tag -v --format="tagname : %(tag)" "signed-tag" >actual && + test_cmp expect actual +' + +test_expect_success 'verifying a forged tag with --format fail and format accordingly' ' + cat >expect <<-\EOF && + tagname : forged-tag + EOF + test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" >actual && + test_cmp expect actual +' + # blank and empty messages for signed tags: get_tag_header empty-signed-tag $commit commit $time >expect -- 2.10.0
[PATCH v4 0/7] Add --format to tag verification
From: Santiago Torres This is the fourth iteration of the series in [1][2][3], which comes as a result of the discussion in [4]. The main goal of this patch series is to bring --format to git tag verification so that upper-layer tools can inspect the content of a tag and make decisions based on those contents. In this re-woll we: * Fixed the author fields and signed off by's throughout the patch series * Added two more patches with unit tests to ensure the format specifier behaves as expected * Fixed a missing initialization for the format specifier in verify-tag which caused a crash. * Fixed an outdated git commit message that had the previous name of a function definition. Thanks, -Santiago [1] http://public-inbox.org/git/20160930221806.3398-1-santi...@nyu.edu/ [2] http://public-inbox.org/git/20160922185317.349-1-santi...@nyu.edu/ [3] http://public-inbox.org/git/20160926224233.32702-1-santi...@nyu.edu/ [4] http://public-inbox.org/git/20160607195608.16643-1-santi...@nyu.edu/ Lukas Puehringer (4): tag: add format specifier to gpg_verify_tag gpg-interface, tag: add GPG_VERIFY_QUIET flag ref-filter: add function to print single ref_array_item builtin/tag: add --format argument for tag -v Santiago Torres (3): builtin/verify-tag: add --format to verify-tag t/t7030-verify-tag: Add --format specifier tests t/t7004-tag: Add --format specifier tests Documentation/git-tag.txt| 2 +- Documentation/git-verify-tag.txt | 2 +- builtin/tag.c| 34 +++--- builtin/verify-tag.c | 13 +++-- gpg-interface.h | 1 + ref-filter.c | 10 ++ ref-filter.h | 3 +++ t/t7004-tag.sh | 16 t/t7030-verify-tag.sh| 16 tag.c| 22 +++--- tag.h| 4 ++-- 11 files changed, 99 insertions(+), 24 deletions(-) -- 2.10.0
[PATCH v4 1/7] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: Lukas Puehringer Functions that print git object information may require that the gpg-interface functions be silent. Add GPG_VERIFY_QUIET flag and prevent print_signature_buffer from being called if flag is set. Signed-off-by: Lukas Puehringer --- gpg-interface.h | 1 + tag.c | 5 - 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/gpg-interface.h b/gpg-interface.h index ea68885..85dc982 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -3,6 +3,7 @@ #define GPG_VERIFY_VERBOSE 1 #define GPG_VERIFY_RAW 2 +#define GPG_VERIFY_QUIET 4 struct signature_check { char *payload; diff --git a/tag.c b/tag.c index d1dcd18..291073f 100644 --- a/tag.c +++ b/tag.c @@ -3,6 +3,7 @@ #include "commit.h" #include "tree.h" #include "blob.h" +#include "gpg-interface.h" const char *tag_type = "tag"; @@ -24,7 +25,9 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags) ret = check_signature(buf, payload_size, buf + payload_size, size - payload_size, &sigc); - print_signature_buffer(&sigc, flags); + + if (!(flags & GPG_VERIFY_QUIET)) + print_signature_buffer(&sigc, flags); signature_check_clear(&sigc); return ret; -- 2.10.0
[PATCH v4 2/7] ref-filter: add function to print single ref_array_item
From: Lukas Puehringer ref-filter functions are useful for printing git object information using a format specifier. However, some other modules may not want to use this functionality on a ref-array but only print a single item. Expose a pretty_print_ref function to create, pretty print and free individual ref-items. Signed-off-by: Lukas Puehringer --- ref-filter.c | 10 ++ ref-filter.h | 3 +++ 2 files changed, 13 insertions(+) diff --git a/ref-filter.c b/ref-filter.c index 9adbb8a..cfbcd73 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1637,6 +1637,16 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu putchar('\n'); } +void pretty_print_ref(const char *name, const unsigned char *sha1, + const char *format, unsigned kind) +{ + struct ref_array_item *ref_item; + ref_item = new_ref_array_item(name, sha1, 0); + ref_item->kind = kind; + show_ref_array_item(ref_item, format, 0); + free_array_item(ref_item); +} + /* If no sorting option is given, use refname to sort as default */ struct ref_sorting *ref_default_sorting(void) { diff --git a/ref-filter.h b/ref-filter.h index 14d435e..3d23090 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -107,4 +107,7 @@ struct ref_sorting *ref_default_sorting(void); /* Function to parse --merged and --no-merged options */ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset); +void pretty_print_ref(const char *name, const unsigned char *sha1, + const char *format, unsigned kind); + #endif /* REF_FILTER_H */ -- 2.10.0
[PATCH v4 3/7] tag: add format specifier to gpg_verify_tag
From: Lukas Puehringer Calling functions for gpg_verify_tag() may desire to print relevant information about the header for further verification. Add an optional format argument to print any desired information after GPG verification. Signed-off-by: Lukas Puehringer --- builtin/tag.c| 2 +- builtin/verify-tag.c | 2 +- tag.c| 17 +++-- tag.h| 4 ++-- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index 50e4ae5..14f3b48 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -105,7 +105,7 @@ static int delete_tag(const char *name, const char *ref, static int verify_tag(const char *name, const char *ref, const unsigned char *sha1) { - return gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE); + return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE); } static int do_sign(struct strbuf *buffer) diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index 99f8148..de10198 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -51,7 +51,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) const char *name = argv[i++]; if (get_sha1(name, sha1)) had_error = !!error("tag '%s' not found.", name); - else if (gpg_verify_tag(sha1, name, flags)) + else if (verify_and_format_tag(sha1, name, NULL, flags)) had_error = 1; } return had_error; diff --git a/tag.c b/tag.c index 291073f..d3512c0 100644 --- a/tag.c +++ b/tag.c @@ -4,6 +4,7 @@ #include "tree.h" #include "blob.h" #include "gpg-interface.h" +#include "ref-filter.h" const char *tag_type = "tag"; @@ -33,8 +34,8 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags) return ret; } -int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report, - unsigned flags) +int verify_and_format_tag(const unsigned char *sha1, const char *name, + const char *fmt_pretty, unsigned flags) { enum object_type type; char *buf; @@ -44,21 +45,25 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report, type = sha1_object_info(sha1, NULL); if (type != OBJ_TAG) return error("%s: cannot verify a non-tag object of type %s.", - name_to_report ? - name_to_report : + name ? + name : find_unique_abbrev(sha1, DEFAULT_ABBREV), typename(type)); buf = read_sha1_file(sha1, &type, &size); if (!buf) return error("%s: unable to read file.", - name_to_report ? - name_to_report : + name ? + name : find_unique_abbrev(sha1, DEFAULT_ABBREV)); ret = run_gpg_verify(buf, size, flags); free(buf); + + if (fmt_pretty) + pretty_print_ref(name, sha1, fmt_pretty, FILTER_REFS_TAGS); + return ret; } diff --git a/tag.h b/tag.h index a5721b6..896b9c2 100644 --- a/tag.h +++ b/tag.h @@ -17,7 +17,7 @@ extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long si extern int parse_tag(struct tag *item); extern struct object *deref_tag(struct object *, const char *, int); extern struct object *deref_tag_noverify(struct object *); -extern int gpg_verify_tag(const unsigned char *sha1, - const char *name_to_report, unsigned flags); +extern int verify_and_format_tag(const unsigned char *sha1, const char *name, + const char *fmt_pretty, unsigned flags); #endif /* TAG_H */ -- 2.10.0
[PATCH v4 4/7] builtin/verify-tag: add --format to verify-tag
From: Santiago Torres Callers of verify-tag may want to cross-check the tagname from refs/tags with the tagname from the tag object header upon GPG verification. This is to avoid tag refs that point to an incorrect object. Add a --format parameter to git verify-tag to print the formatted tag object header in addition to or instead of the --verbose or --raw GPG verification output. Signed-off-by: Santiago Torres --- Documentation/git-verify-tag.txt | 2 +- builtin/verify-tag.c | 13 +++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/Documentation/git-verify-tag.txt b/Documentation/git-verify-tag.txt index d590edc..0b8075d 100644 --- a/Documentation/git-verify-tag.txt +++ b/Documentation/git-verify-tag.txt @@ -8,7 +8,7 @@ git-verify-tag - Check the GPG signature of tags SYNOPSIS [verse] -'git verify-tag' ... +'git verify-tag' [--format=] ... DESCRIPTION --- diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index de10198..745b6a6 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -12,12 +12,14 @@ #include #include "parse-options.h" #include "gpg-interface.h" +#include "ref-filter.h" static const char * const verify_tag_usage[] = { - N_("git verify-tag [-v | --verbose] ..."), + N_("git verify-tag [-v | --verbose] [--format=] ..."), NULL }; + static int git_verify_tag_config(const char *var, const char *value, void *cb) { int status = git_gpg_config(var, value, cb); @@ -30,9 +32,11 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) { int i = 1, verbose = 0, had_error = 0; unsigned flags = 0; + char *fmt_pretty = NULL; const struct option verify_tag_options[] = { OPT__VERBOSE(&verbose, N_("print tag contents")), OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW), + OPT_STRING( 0 , "format", &fmt_pretty, N_("format"), N_("format to use for the output")), OPT_END() }; @@ -46,12 +50,17 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) if (verbose) flags |= GPG_VERIFY_VERBOSE; + if (fmt_pretty) { + verify_ref_format(fmt_pretty); + flags |= GPG_VERIFY_QUIET; + } + while (i < argc) { unsigned char sha1[20]; const char *name = argv[i++]; if (get_sha1(name, sha1)) had_error = !!error("tag '%s' not found.", name); - else if (verify_and_format_tag(sha1, name, NULL, flags)) + else if (verify_and_format_tag(sha1, name, fmt_pretty, flags)) had_error = 1; } return had_error; -- 2.10.0
Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2
Am 07.10.2016 um 02:46 schrieb Jeff King: On Tue, Sep 27, 2016 at 09:11:58PM +0200, René Scharfe wrote: Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs instead of taking detours through find_unique_abbrev() and its static buffer. This is shorter and a bit more efficient. [...] diff --git a/diff.c b/diff.c index a178ed3..be11e4e 100644 --- a/diff.c +++ b/diff.c @@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg, } strbuf_addf(msg, "%s%sindex %s..", line_prefix, set, find_unique_abbrev(one->oid.hash, abbrev)); - strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev)); + strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev); if (one->mode == two->mode) strbuf_addf(msg, " %06o", one->mode); strbuf_addf(msg, "%s\n", reset); This one is an interesting case, and maybe a good example of why blind coccinelle usage can have some pitfalls. :) Thank you for paying attention. :) In general I agree that the surrounding code of such changes should be checked; the issue at hand could be part of a bigger problem. We get rid of the strbuf_addstr(), but notice that we leave untouched the find_unique_abbrev() call immediately above. There was a symmetry to the two that has been lost. Probably either: strbuf_addf(msg, "%s%sindex %s..%s", line_prefix, set find_unique_abbrev(one->oid.hash, abbrev), find_unique_abbrev(two->oid.hash, abbrev)); or: strbuf_addf(msg, "%s%sindex ", line_prefix, set); strbuf_add_unique_abbrev(msg, one->oid.hash, abbrev); strbuf_addstr(msg, ".."); strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev); would be a more appropriate refactoring. The problem is in the original patch (which also lacks symmetry; either this predates the multi-buffer find_unique_abbrev, or the original author didn't know about it), but I think your refactor makes it slightly worse. I still think the automatically generated patch is a net win, but we shouldn't stop there. I noticed because I have another series which touches these lines, and it wants to symmetrically swap out find_unique_abbrev for something else. :) I don't think it's a big enough deal to switch now (and I've already rebased my series which will touch these lines), but I wanted to mention it as a thing to watch out for as we do more of these kinds of automated transformations. OK, then I'll wait for that series to land. --- a/submodule.c +++ b/submodule.c @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path, find_unique_abbrev(one->hash, DEFAULT_ABBREV)); if (!fast_backward && !fast_forward) strbuf_addch(&sb, '.'); - strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV)); + strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV); This one is a similar situation, I think. Yes, and there are some more. Will take a look. I don't know how to crack printf-style formats using semantic patches. It's easy for fixed formats (silly example): - strbuf_addf(sb, "%s%s", a, b); + strbuf_addf(sb, "%s", a); + strbuf_addf(sb, "%s", b); But how to do that for arbitrary formats? Probably not worth it.. René
RE: Uninitialized submodules as symlinks
> -Original Message- > From: Stefan Beller [mailto:sbel...@google.com] > Sent: Friday, October 07, 2016 2:56 PM > To: David Turner > Cc: git@vger.kernel.org > Subject: Re: Uninitialized submodules as symlinks > > On Fri, Oct 7, 2016 at 11:17 AM, David Turner > wrote: > > Presently, uninitialized submodules are materialized in the working tree > > as empty directories. > > Right, there has to be something, to hint at the user that creating a file > with that path is probably not what they want. > > > We would like to consider having them be symlinks. Specifically, we'd > > like them to be symlinks into a FUSE filesystem which retrieves files on > > demand. > > > > We've actually already got a FUSE filesystem written, but we use a > > different (semi-manual) means to connect it to the initialized submodules. > > So you currently do a > > git submodule init > custom-submodule make-symlink > > ? We do something like For each initialized submodule: symlink it into the right place in .../somedir For each uninitialized submodule: symlink from the FUSE into the right place in .../somedir So .../somedir has the structure of the git main repo, but is all symlinks -- some into FUSE, some into the git repo. This means that when we initialize (or deinitialize) a submodule, we need to re-run the linking script. > > We hope to release this FUSE filesystem as free software at some point > > soon, but we do not yet have a fixed schedule for doing so. Having to run > > a command to create the symlink-based "union" filesystem is not optimal > > (since we have to re-run it every time we initialize or deinitialize a > > submodule). > > > > But if the uninitialized submodules could be symlinks into the FUSE > > filesystem, we wouldn't have this problem. This solution isn't > > necessarily FUSE-specific -- perhaps someone would want copies of the same > > submodule in multiple repos, and would want to save disk space by having > > all copies point to the same place. So the symlinks would be configured > > by a per-submodule config variable. > > I'd imagine that you want both a per-submodule config variable as well as > a global variable that is a default for all submodules? > > git config submodule.trySymlinkDefault /mounted/fuse/ > # any (new) submodule tries to be linked to /mounted/fuse/ > git config submodule..symlinked ~/my/private/symlinked > # The submodule goes into another path. > > As you propose the FUSE filesystem fetches files on demand, you probably > want to disable things that scan the whole submodule, e.g. look at > submodule..ignore to suppress status looking at all files. I would actually expect that git would detect that the symlink is unmodified from the configured symlink and automatically decide not to look there. > When looking through the options, you could add the value "symlink" to > submodule..update, which then respects the > submodule.trySymlinkDefault if present, such that > > git clone --recurse-submodules ... > > works and sets up the FUSE thing correctly. > > How does the FUSE system handle different versions, i.e. > `git submodule update` to checkout another version of the submodule? > (btw, I plan on working on integrating submodules to "git checkout", so > "submodule update" would not need to be run there, but we'd hook it into > checkout instead) The fuse has a (virtual) directory for each SHA of the main repo, with each submodule mapped to the then-current version of the submodule's code. Actually, it's a bit more complicated because the uninitialized modules point to already-built binaries -- that is, the symlink is to something like $fuse/$SHA/built/$submodule. If you check out a new version of the main module, in our current setup, you need to again update all of the submodule symlinks (as described above). Under my proposal, I guess this would still need to happen. A post-checkout hook could handle it either way. Despite this flaw, switching a submodule between an initialized and deinitialized state would still be more seamless with the symlinks. > > Naturally, this would require some changes to code that examines the > working tree -- git status, git diff, etc. They would have to report > "unchanged" for submodules which were still symlinks to the configured > location. I have not yet looked at the implementation details beyond > this. > > > > Does this idea make any sense? If I were to implement it (probably in a > few months, but no official timeline yet), would patches be considered? > > I am happy to review patches. Thanks.
Re: Uninitialized submodules as symlinks
On Fri, Oct 7, 2016 at 11:17 AM, David Turner wrote: > Presently, uninitialized submodules are materialized in the working tree as > empty directories. Right, there has to be something, to hint at the user that creating a file with that path is probably not what they want. > We would like to consider having them be symlinks. Specifically, we'd like > them to be symlinks into a FUSE filesystem which retrieves files on demand. > > We've actually already got a FUSE filesystem written, but we use a different > (semi-manual) means to connect it to the initialized submodules. So you currently do a git submodule init custom-submodule make-symlink ? > We hope to release this FUSE filesystem as free software at some point soon, > but we do not yet have a fixed schedule for doing so. Having to run a > command to create the symlink-based "union" filesystem is not optimal (since > we have to re-run it every time we initialize or deinitialize a submodule). > > But if the uninitialized submodules could be symlinks into the FUSE > filesystem, we wouldn't have this problem. This solution isn't necessarily > FUSE-specific -- perhaps someone would want copies of the same submodule in > multiple repos, and would want to save disk space by having all copies point > to the same place. So the symlinks would be configured by a per-submodule > config variable. I'd imagine that you want both a per-submodule config variable as well as a global variable that is a default for all submodules? git config submodule.trySymlinkDefault /mounted/fuse/ # any (new) submodule tries to be linked to /mounted/fuse/ git config submodule..symlinked ~/my/private/symlinked # The submodule goes into another path. As you propose the FUSE filesystem fetches files on demand, you probably want to disable things that scan the whole submodule, e.g. look at submodule..ignore to suppress status looking at all files. When looking through the options, you could add the value "symlink" to submodule..update, which then respects the submodule.trySymlinkDefault if present, such that git clone --recurse-submodules ... works and sets up the FUSE thing correctly. How does the FUSE system handle different versions, i.e. `git submodule update` to checkout another version of the submodule? (btw, I plan on working on integrating submodules to "git checkout", so "submodule update" would not need to be run there, but we'd hook it into checkout instead) > > Naturally, this would require some changes to code that examines the working > tree -- git status, git diff, etc. They would have to report "unchanged" for > submodules which were still symlinks to the configured location. I have not > yet looked at the implementation details beyond this. > > Does this idea make any sense? If I were to implement it (probably in a few > months, but no official timeline yet), would patches be considered? I am happy to review patches.
Re: "Purposes, Concepts,Misfits, and a Redesign of Git" (a research paper)
W dniu 07.10.2016 o 19:52, Konstantin Khomoutov pisze: > On Fri, 30 Sep 2016 19:14:13 +0300 > Konstantin Khomoutov wrote: > >> The "It Will Never Work in Theory" blog has just posted a summary of a >> study which tried to identify shortcomings in the design of Git. >> >> In the hope it might be interesting, I post this summary here. >> URL: http://neverworkintheory.org/2016/09/30/rethinking-git.html > > I think it would be cool if someone among subscribers could post a link > to our discussion thread [2] back onto that page. Unfortunatrly that > requires a Disqus login which I don't have, so may be someone in > possession of such login could do that? ;-) > > 2. > https://public-inbox.org/git/ce42f934-4a94-fa29-cff0-5ebb0f004...@gmail.com/T/#e95875b7940512b432ab2e29b3dd50ca448df9720 Posted. Thanks for the idea. -- Jakub Narębski
Re: [PATCH v7 0/4] recursive support for ls-files
On 10/07, Stefan Beller wrote: > On Fri, Oct 7, 2016 at 11:34 AM, Stefan Beller wrote: > > On Fri, Oct 7, 2016 at 11:18 AM, Brandon Williams wrote: > >> Few minor fixes pointed out Stefan > > > > The series itself looks good though. :) > > Thanks, > Stefan Thanks! And I'll keep that in mind for the future. Still lots to learn about contributing to the project. -- Brandon Williams
Re: [PATCH v7 0/4] recursive support for ls-files
On Fri, Oct 7, 2016 at 11:34 AM, Stefan Beller wrote: > On Fri, Oct 7, 2016 at 11:18 AM, Brandon Williams wrote: >> Few minor fixes pointed out Stefan > The series itself looks good though. :) Thanks, Stefan
Re: [PATCH v7 0/4] recursive support for ls-files
On Fri, Oct 7, 2016 at 11:18 AM, Brandon Williams wrote: > Few minor fixes pointed out Stefan Nit: This is not very informative, so you could provide an "interdiff" to the last patch, (see https://github.com/trast/tbdiff for a sophisticated tool, I usually do a git diff origin/sb/) That helps reviewers as they do not need to review it all again, but they may remember what they annotated and see how you addressed it. Thanks, Stefan
Re: [PATCH v2 3/3] batch check whether submodule needs pushing into one call
On Fri, Oct 7, 2016 at 8:06 AM, Heiko Voigt wrote: > We run a command for each sha1 change in a submodule. This is > unnecessary since we can simply batch all sha1's we want to check into > one command. Lets do it so we can speedup the check when many submodule > changes are in need of checking. > > Signed-off-by: Heiko Voigt > --- > submodule.c | 63 > + > 1 file changed, 34 insertions(+), 29 deletions(-) > > diff --git a/submodule.c b/submodule.c > index 5044afc2f8..a05c2a34b1 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -529,27 +529,49 @@ static int append_hash_to_argv(const unsigned char > sha1[20], void *data) > return 0; > } > > -static int submodule_needs_pushing(const char *path, const unsigned char > sha1[20]) > +static int check_has_hash(const unsigned char sha1[20], void *data) > { > - if (add_submodule_odb(path) || !lookup_commit_reference(sha1)) > + int *has_hash = (int *) data; > + > + if (!lookup_commit_reference(sha1)) > + *has_hash = 0; > + > + return 0; > +} > + > +static int submodule_has_hashes(const char *path, struct sha1_array *hashes) > +{ > + int has_hash = 1; > + > + if (add_submodule_odb(path)) > + return 0; > + > + sha1_array_for_each_unique(hashes, check_has_hash, &has_hash); > + return has_hash; > +} > + > +static int submodule_needs_pushing(const char *path, struct sha1_array > *hashes) > +{ > + if (!submodule_has_hashes(path, hashes)) So the above is an implicit lookup already, but we did that before, too, so it's fine. > @@ -658,13 +665,11 @@ int find_unpushed_submodules(struct sha1_array *hashes, > argv_array_clear(&argv); > > for (i = 0; i < submodules.nr; i++) { > - struct string_list_item *item = &submodules.items[i]; > - struct collect_submodule_from_sha1s_data data; > - data.submodule_path = item->string; > - data.needs_pushing = needs_pushing; > - sha1_array_for_each_unique((struct sha1_array *) item->util, > - collect_submodules_from_sha1s, > - &data); > + struct string_list_item *submodule = &submodules.items[i]; > + struct sha1_array *hashes = (struct sha1_array *) > submodule->util; > + > + if (submodule_needs_pushing(submodule->string, hashes)) > + string_list_insert(needs_pushing, submodule->string); That makes sense. Thanks! Stefan
[PATCH v7 3/4] ls-files: pass through safe options for --recurse-submodules
Pass through some known-safe options when recursing into submodules. (--cached, -v, -t, -z, --debug, --eol) Signed-off-by: Brandon Williams --- builtin/ls-files.c | 30 +++--- t/t3007-ls-files-recurse-submodules.sh | 16 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/builtin/ls-files.c b/builtin/ls-files.c index 63befed..b6144a5 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -30,6 +30,7 @@ static int line_terminator = '\n'; static int debug_mode; static int show_eol; static int recurse_submodules; +static struct argv_array submodules_options = ARGV_ARRAY_INIT; static const char *prefix; static const char *super_prefix; @@ -168,6 +169,25 @@ static void show_killed_files(struct dir_struct *dir) } } +/* + * Compile an argv_array with all of the options supported by --recurse_submodules + */ +static void compile_submodule_options(const struct dir_struct *dir, int show_tag) +{ + if (line_terminator == '\0') + argv_array_push(&submodules_options, "-z"); + if (show_tag) + argv_array_push(&submodules_options, "-t"); + if (show_valid_bit) + argv_array_push(&submodules_options, "-v"); + if (show_cached) + argv_array_push(&submodules_options, "--cached"); + if (show_eol) + argv_array_push(&submodules_options, "--eol"); + if (debug_mode) + argv_array_push(&submodules_options, "--debug"); +} + /** * Recursively call ls-files on a submodule */ @@ -182,6 +202,9 @@ static void show_gitlink(const struct cache_entry *ce) argv_array_push(&cp.args, "ls-files"); argv_array_push(&cp.args, "--recurse-submodules"); + /* add supported options */ + argv_array_pushv(&cp.args, submodules_options.argv); + cp.git_cmd = 1; cp.dir = ce->name; status = run_command(&cp); @@ -567,11 +590,12 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) if (require_work_tree && !is_inside_work_tree()) setup_work_tree(); + if (recurse_submodules) + compile_submodule_options(&dir, show_tag); + if (recurse_submodules && (show_stage || show_deleted || show_others || show_unmerged || -show_killed || show_modified || show_resolve_undo || -show_valid_bit || show_tag || show_eol || with_tree || -(line_terminator == '\0'))) +show_killed || show_modified || show_resolve_undo || with_tree)) die("ls-files --recurse-submodules unsupported mode"); if (recurse_submodules && error_unmatch) diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh index b5a53c3..33a2ea7 100755 --- a/t/t3007-ls-files-recurse-submodules.sh +++ b/t/t3007-ls-files-recurse-submodules.sh @@ -34,6 +34,18 @@ test_expect_success 'ls-files correctly outputs files in submodule' ' test_cmp expect actual ' +test_expect_success 'ls-files correctly outputs files in submodule with -z' ' + lf_to_nul >expect <<-\EOF && + .gitmodules + a + b/b + submodule/c + EOF + + git ls-files --recurse-submodules -z >actual && + test_cmp expect actual +' + test_expect_success 'ls-files does not output files not added to a repo' ' cat >expect <<-\EOF && .gitmodules @@ -86,15 +98,11 @@ test_incompatible_with_recurse_submodules () { " } -test_incompatible_with_recurse_submodules -z -test_incompatible_with_recurse_submodules -v -test_incompatible_with_recurse_submodules -t test_incompatible_with_recurse_submodules --deleted test_incompatible_with_recurse_submodules --modified test_incompatible_with_recurse_submodules --others test_incompatible_with_recurse_submodules --stage test_incompatible_with_recurse_submodules --killed test_incompatible_with_recurse_submodules --unmerged -test_incompatible_with_recurse_submodules --eol test_done -- 2.10.0
[PATCH v7 4/4] ls-files: add pathspec matching for submodules
Pathspecs can be a bit tricky when trying to apply them to submodules. The main challenge is that the pathspecs will be with respect to the superproject and not with respect to paths in the submodule. The approach this patch takes is to pass in the identical pathspec from the superproject to the submodule in addition to the submodule-prefix, which is the path from the root of the superproject to the submodule, and then we can compare an entry in the submodule prepended with the submodule-prefix to the pathspec in order to determine if there is a match. This patch also permits the pathspec logic to perform a prefix match against submodules since a pathspec could refer to a file inside of a submodule. Due to limitations in the wildmatch logic, a prefix match is only done literally. If any wildcard character is encountered we'll simply punt and produce a false positive match. More accurate matching will be done once inside the submodule. This is due to the superproject not knowing what files could exist in the submodule. Signed-off-by: Brandon Williams --- Documentation/git-ls-files.txt | 3 +- builtin/ls-files.c | 27 +++-- dir.c | 46 +- dir.h | 4 ++ t/t3007-ls-files-recurse-submodules.sh | 108 - 5 files changed, 175 insertions(+), 13 deletions(-) diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt index ea01d45..446209e 100644 --- a/Documentation/git-ls-files.txt +++ b/Documentation/git-ls-files.txt @@ -140,8 +140,7 @@ a space) at the start of each line: --recurse-submodules:: Recursively calls ls-files on each submodule in the repository. - Currently there is only support for the --cached mode without a - pathspec. + Currently there is only support for the --cached mode. --abbrev[=]:: Instead of showing the full 40-byte hexadecimal object diff --git a/builtin/ls-files.c b/builtin/ls-files.c index b6144a5..0f25914 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -195,6 +195,7 @@ static void show_gitlink(const struct cache_entry *ce) { struct child_process cp = CHILD_PROCESS_INIT; int status; + int i; argv_array_pushf(&cp.args, "--super-prefix=%s%s/", super_prefix ? super_prefix : "", @@ -205,6 +206,15 @@ static void show_gitlink(const struct cache_entry *ce) /* add supported options */ argv_array_pushv(&cp.args, submodules_options.argv); + /* +* Pass in the original pathspec args. The submodule will be +* responsible for prepending the 'submodule_prefix' prior to comparing +* against the pathspec for matches. +*/ + argv_array_push(&cp.args, "--"); + for (i = 0; i < pathspec.nr; i++) + argv_array_push(&cp.args, pathspec.items[i].original); + cp.git_cmd = 1; cp.dir = ce->name; status = run_command(&cp); @@ -223,7 +233,8 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce) if (len >= ce_namelen(ce)) die("git ls-files: internal error - cache entry not superset of prefix"); - if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) { + if (recurse_submodules && S_ISGITLINK(ce->ce_mode) && + submodule_path_match(&pathspec, name.buf, ps_matched)) { show_gitlink(ce); } else if (match_pathspec(&pathspec, name.buf, name.len, len, ps_matched, @@ -602,16 +613,20 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) die("ls-files --recurse-submodules does not support " "--error-unmatch"); - if (recurse_submodules && argc) - die("ls-files --recurse-submodules does not support pathspec"); - parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD | PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP, prefix, argv); - /* Find common prefix for all pathspec's */ - max_prefix = common_prefix(&pathspec); + /* +* Find common prefix for all pathspec's +* This is used as a performance optimization which unfortunately cannot +* be done when recursing into submodules +*/ + if (recurse_submodules) + max_prefix = NULL; + else + max_prefix = common_prefix(&pathspec); max_prefix_len = max_prefix ? strlen(max_prefix) : 0; /* Treat unmatching pathspec elements as errors */ diff --git a/dir.c b/dir.c index 0ea235f..28e9736 100644 --- a/dir.c +++ b/dir.c @@ -207,8 +207,9 @@ int within_depth(const char *name, int namelen, return 1; } -#define DO_MATCH_EXCLUDE 1 -#define DO_MATCH_DIRECTORY 2 +#define DO_MATCH_EXCLUDE (1<<0) +#define DO_MATCH_DIRECTORY (1<<1) +#define DO_M
[PATCH v7 2/4] ls-files: optionally recurse into submodules
Allow ls-files to recognize submodules in order to retrieve a list of files from a repository's submodules. This is done by forking off a process to recursively call ls-files on all submodules. Use top-level --super-prefix option to pass a path to the submodule which it can use to prepend to output or pathspec matching logic. Signed-off-by: Brandon Williams --- Documentation/git-ls-files.txt | 8 +- builtin/ls-files.c | 138 - git.c | 2 +- t/t3007-ls-files-recurse-submodules.sh | 100 4 files changed, 208 insertions(+), 40 deletions(-) create mode 100755 t/t3007-ls-files-recurse-submodules.sh diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt index 0d933ac..ea01d45 100644 --- a/Documentation/git-ls-files.txt +++ b/Documentation/git-ls-files.txt @@ -18,7 +18,8 @@ SYNOPSIS [--exclude-per-directory=] [--exclude-standard] [--error-unmatch] [--with-tree=] - [--full-name] [--abbrev] [--] [...] + [--full-name] [--recurse-submodules] + [--abbrev] [--] [...] DESCRIPTION --- @@ -137,6 +138,11 @@ a space) at the start of each line: option forces paths to be output relative to the project top directory. +--recurse-submodules:: + Recursively calls ls-files on each submodule in the repository. + Currently there is only support for the --cached mode without a + pathspec. + --abbrev[=]:: Instead of showing the full 40-byte hexadecimal object lines, show only a partial prefix. diff --git a/builtin/ls-files.c b/builtin/ls-files.c index 00ea91a..63befed 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -14,6 +14,7 @@ #include "resolve-undo.h" #include "string-list.h" #include "pathspec.h" +#include "run-command.h" static int abbrev; static int show_deleted; @@ -28,8 +29,10 @@ static int show_valid_bit; static int line_terminator = '\n'; static int debug_mode; static int show_eol; +static int recurse_submodules; static const char *prefix; +static const char *super_prefix; static int max_prefix_len; static int prefix_len; static struct pathspec pathspec; @@ -68,11 +71,24 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path) static void write_name(const char *name) { /* +* Prepend the super_prefix to name to construct the full_name to be +* written. +*/ + struct strbuf full_name = STRBUF_INIT; + if (super_prefix) { + strbuf_addstr(&full_name, super_prefix); + strbuf_addstr(&full_name, name); + name = full_name.buf; + } + + /* * With "--full-name", prefix_len=0; this caller needs to pass * an empty string in that case (a NULL is good for ""). */ write_name_quoted_relative(name, prefix_len ? prefix : NULL, stdout, line_terminator); + + strbuf_release(&full_name); } static void show_dir_entry(const char *tag, struct dir_entry *ent) @@ -152,55 +168,84 @@ static void show_killed_files(struct dir_struct *dir) } } +/** + * Recursively call ls-files on a submodule + */ +static void show_gitlink(const struct cache_entry *ce) +{ + struct child_process cp = CHILD_PROCESS_INIT; + int status; + + argv_array_pushf(&cp.args, "--super-prefix=%s%s/", +super_prefix ? super_prefix : "", +ce->name); + argv_array_push(&cp.args, "ls-files"); + argv_array_push(&cp.args, "--recurse-submodules"); + + cp.git_cmd = 1; + cp.dir = ce->name; + status = run_command(&cp); + if (status) + exit(status); +} + static void show_ce_entry(const char *tag, const struct cache_entry *ce) { + struct strbuf name = STRBUF_INIT; int len = max_prefix_len; + if (super_prefix) + strbuf_addstr(&name, super_prefix); + strbuf_addstr(&name, ce->name); if (len >= ce_namelen(ce)) die("git ls-files: internal error - cache entry not superset of prefix"); - if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce), - len, ps_matched, - S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode))) - return; + if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) { + show_gitlink(ce); + } else if (match_pathspec(&pathspec, name.buf, name.len, + len, ps_matched, + S_ISDIR(ce->ce_mode) || + S_ISGITLINK(ce->ce_mode))) { + if (tag && *tag && show_valid_bit && + (ce->ce_flags & CE_VALID)) { + static char alttag[4]; +
[PATCH v7 1/4] git: make super-prefix option
Add a super-prefix environment variable 'GIT_INTERNAL_SUPER_PREFIX' which can be used to specify a path from above a repository down to its root. When such a super-prefix is specified, the paths reported by Git are prefixed with it to make them relative to that directory "above". The paths given by the user on the command line (e.g. "git subcmd --output-file=path/to/a/file" and pathspecs) are taken relative to the directory "above" to match. The immediate use of this option is by commands which have a --recurse-submodule option in order to give context to submodules about how they were invoked. This option is currently only allowed for builtins which support a super-prefix. Signed-off-by: Brandon Williams --- Documentation/git.txt | 6 ++ cache.h | 2 ++ environment.c | 13 + git.c | 25 + 4 files changed, 46 insertions(+) diff --git a/Documentation/git.txt b/Documentation/git.txt index 7913fc2..2188ae6 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -13,6 +13,7 @@ SYNOPSIS [--exec-path[=]] [--html-path] [--man-path] [--info-path] [-p|--paginate|--no-pager] [--no-replace-objects] [--bare] [--git-dir=] [--work-tree=] [--namespace=] +[--super-prefix=] [] DESCRIPTION @@ -601,6 +602,11 @@ foo.bar= ...`) sets `foo.bar` to the empty string. details. Equivalent to setting the `GIT_NAMESPACE` environment variable. +--super-prefix=:: + Currently for internal use only. Set a prefix which gives a path from + above a repository down to its root. One use is to give submodules + context about the superproject that invoked it. + --bare:: Treat the repository as a bare repository. If GIT_DIR environment is not set, it is set to the current working diff --git a/cache.h b/cache.h index 3556326..8cf495d 100644 --- a/cache.h +++ b/cache.h @@ -408,6 +408,7 @@ static inline enum object_type object_type(unsigned int mode) #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE" #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE" #define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX" +#define GIT_SUPER_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUPER_PREFIX" #define DEFAULT_GIT_DIR_ENVIRONMENT ".git" #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY" #define INDEX_ENVIRONMENT "GIT_INDEX_FILE" @@ -468,6 +469,7 @@ extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir); extern int get_common_dir(struct strbuf *sb, const char *gitdir); extern const char *get_git_namespace(void); extern const char *strip_namespace(const char *namespaced_ref); +extern const char *get_super_prefix(void); extern const char *get_git_work_tree(void); /* diff --git a/environment.c b/environment.c index ca72464..d12d7db 100644 --- a/environment.c +++ b/environment.c @@ -100,6 +100,8 @@ static char *work_tree; static const char *namespace; static size_t namespace_len; +static const char *super_prefix; + static const char *git_dir, *git_common_dir; static char *git_object_dir, *git_index_file, *git_graft_file; int git_db_env, git_index_env, git_graft_env, git_common_dir_env; @@ -120,6 +122,7 @@ const char * const local_repo_env[] = { NO_REPLACE_OBJECTS_ENVIRONMENT, GIT_REPLACE_REF_BASE_ENVIRONMENT, GIT_PREFIX_ENVIRONMENT, + GIT_SUPER_PREFIX_ENVIRONMENT, GIT_SHALLOW_FILE_ENVIRONMENT, GIT_COMMON_DIR_ENVIRONMENT, NULL @@ -222,6 +225,16 @@ const char *strip_namespace(const char *namespaced_ref) return namespaced_ref + namespace_len; } +const char *get_super_prefix(void) +{ + static int initialized; + if (!initialized) { + super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT); + initialized = 1; + } + return super_prefix; +} + static int git_work_tree_initialized; /* diff --git a/git.c b/git.c index 1c61151..469a83f 100644 --- a/git.c +++ b/git.c @@ -164,6 +164,20 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) setenv(GIT_WORK_TREE_ENVIRONMENT, cmd, 1); if (envchanged) *envchanged = 1; + } else if (!strcmp(cmd, "--super-prefix")) { + if (*argc < 2) { + fprintf(stderr, "No prefix given for --super-prefix.\n" ); + usage(git_usage_string); + } + setenv(GIT_SUPER_PREFIX_ENVIRONMENT, (*argv)[1], 1); + if (envchanged) + *envchanged = 1; + (*argv)++; + (*argc)--; + } else if (skip_prefix(cmd, "--super-prefix=", &cmd)) { + setenv(GIT_SUPER_PREFIX_ENVIRONMENT, cmd, 1); + if (envchanged) + *envchanged = 1; } else if (!strcmp(c
[PATCH v7 0/4] recursive support for ls-files
Few minor fixes pointed out Stefan Brandon Williams (4): git: make super-prefix option ls-files: optionally recurse into submodules ls-files: pass through safe options for --recurse-submodules ls-files: add pathspec matching for submodules Documentation/git-ls-files.txt | 7 +- Documentation/git.txt | 6 + builtin/ls-files.c | 181 +--- cache.h| 2 + dir.c | 46 +++- dir.h | 4 + environment.c | 13 ++ git.c | 27 - t/t3007-ls-files-recurse-submodules.sh | 210 + 9 files changed, 452 insertions(+), 44 deletions(-) create mode 100755 t/t3007-ls-files-recurse-submodules.sh -- 2.10.0
Re: [PATCH v2 2/3] serialize collection of refs that contain submodule changes
On Fri, Oct 7, 2016 at 8:06 AM, Heiko Voigt wrote: > We are iterating over each pushed ref and want to check whether it > contains changes to submodules. Instead of immediately checking each ref > lets first collect them and then do the check for all of them in one > revision walk. > > Signed-off-by: Heiko Voigt > --- > submodule.c | 36 +--- > submodule.h | 5 +++-- > transport.c | 29 + > 3 files changed, 45 insertions(+), 25 deletions(-) > > diff --git a/submodule.c b/submodule.c > index 59c9d15905..5044afc2f8 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -522,6 +522,13 @@ static int has_remote(const char *refname, const struct > object_id *oid, > return 1; > } > > +static int append_hash_to_argv(const unsigned char sha1[20], void *data) > +{ > + struct argv_array *argv = (struct argv_array *) data; > + argv_array_push(argv, sha1_to_hex(sha1)); Nit of the day: When using the struct child-process, we have the oldstyle argv NULL terminated array as well as the new style args argv_array. So in that context we'd prefer `args` as a name for argv_array as that helps to distinguish from the old array type. Here however `argv` seems to be a reasonable name, in fact whenever we do not deal with child processes, we seem to not like the `args` name: $ git grep argv_array |wc -l 577 $ git grep argv_array |grep args |wc -l 293 The rest looks good to me. :)
Uninitialized submodules as symlinks
Presently, uninitialized submodules are materialized in the working tree as empty directories. We would like to consider having them be symlinks. Specifically, we'd like them to be symlinks into a FUSE filesystem which retrieves files on demand. We've actually already got a FUSE filesystem written, but we use a different (semi-manual) means to connect it to the initialized submodules. We hope to release this FUSE filesystem as free software at some point soon, but we do not yet have a fixed schedule for doing so. Having to run a command to create the symlink-based "union" filesystem is not optimal (since we have to re-run it every time we initialize or deinitialize a submodule). But if the uninitialized submodules could be symlinks into the FUSE filesystem, we wouldn't have this problem. This solution isn't necessarily FUSE-specific -- perhaps someone would want copies of the same submodule in multiple repos, and would want to save disk space by having all copies point to the same place. So the symlinks would be configured by a per-submodule config variable. Naturally, this would require some changes to code that examines the working tree -- git status, git diff, etc. They would have to report "unchanged" for submodules which were still symlinks to the configured location. I have not yet looked at the implementation details beyond this. Does this idea make any sense? If I were to implement it (probably in a few months, but no official timeline yet), would patches be considered?
Re: [PATCH v2 1/3] serialize collection of changed submodules
On Fri, Oct 7, 2016 at 8:06 AM, Heiko Voigt wrote: > To check whether a submodule needs to be pushed we need to collect all > changed submodules. Lets collect them first and then execute the > possibly expensive test whether certain revisions are already pushed > only once per submodule. > > There is further potential for optimization since we can assemble one > command and only issued that instead of one call for each remote ref in > the submodule. > > Signed-off-by: Heiko Voigt > --- > submodule.c | 63 > - > 1 file changed, 58 insertions(+), 5 deletions(-) > > diff --git a/submodule.c b/submodule.c > index 2de06a3351..59c9d15905 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -554,19 +554,34 @@ static int submodule_needs_pushing(const char *path, > const unsigned char sha1[20 > return 0; > } > > +static struct sha1_array *get_sha1s_from_list(struct string_list *submodules, > + const char *path) So this will take the stringlist `submodules` and insert the path into it, if it wasn't already in there. In case it is newly inserted, add a sha1_array as util, so each inserted path has it's own empty array. So it is both init of the data structures as well as retrieving them. I was initially confused by the name as I assumed it would give you sha1s out of a string list (e.g. transform strings to internal sha1 things). Maybe it's just me having a hard time to understand that, but I feel like the name could be improved. lookup_sha1_list_by_path, insert_path_and_return_sha1_list ? > +{ > + struct string_list_item *item; > + > + item = string_list_insert(submodules, path); > + if (item->util) > + return (struct sha1_array *) item->util; > + > + /* NEEDSWORK: should we have sha1_array_init()? */ > + item->util = xcalloc(1, sizeof(struct sha1_array)); > + return (struct sha1_array *) item->util; > +} > + > static void collect_submodules_from_diff(struct diff_queue_struct *q, > struct diff_options *options, > void *data) > { > int i; > - struct string_list *needs_pushing = data; > + struct string_list *submodules = data; > > for (i = 0; i < q->nr; i++) { > struct diff_filepair *p = q->queue[i]; > + struct sha1_array *hashes; > if (!S_ISGITLINK(p->two->mode)) > continue; > - if (submodule_needs_pushing(p->two->path, p->two->oid.hash)) > - string_list_insert(needs_pushing, p->two->path); > + hashes = get_sha1s_from_list(submodules, p->two->path); > + sha1_array_append(hashes, p->two->oid.hash); > } > } > > @@ -582,14 +597,41 @@ static void find_unpushed_submodule_commits(struct > commit *commit, > diff_tree_combined_merge(commit, 1, &rev); > } > > +struct collect_submodule_from_sha1s_data { > + char *submodule_path; > + struct string_list *needs_pushing; > +}; > + > +static void collect_submodules_from_sha1s(const unsigned char sha1[20], > + void *data) > +{ > + struct collect_submodule_from_sha1s_data *me = > + (struct collect_submodule_from_sha1s_data *) data; > + > + if (submodule_needs_pushing(me->submodule_path, sha1)) > + string_list_insert(me->needs_pushing, me->submodule_path); > +} > + > +static void free_submodules_sha1s(struct string_list *submodules) > +{ > + int i; > + for (i = 0; i < submodules->nr; i++) { > + struct string_list_item *item = &submodules->items[i]; You do not seem to make use of `i` explicitely, so for_each_string_list_item might be more readable here? > + struct sha1_array *hashes = (struct sha1_array *) item->util; > + sha1_array_clear(hashes); > + } > + string_list_clear(submodules, 1); > +} > + > int find_unpushed_submodules(unsigned char new_sha1[20], > const char *remotes_name, struct string_list *needs_pushing) > { > struct rev_info rev; > struct commit *commit; > const char *argv[] = {NULL, NULL, "--not", "NULL", NULL}; > - int argc = ARRAY_SIZE(argv) - 1; > + int argc = ARRAY_SIZE(argv) - 1, i; > char *sha1_copy; > + struct string_list submodules = STRING_LIST_INIT_DUP; > > struct strbuf remotes_arg = STRBUF_INIT; > > @@ -603,12 +645,23 @@ int find_unpushed_submodules(unsigned char new_sha1[20], > die("revision walk setup failed"); > > while ((commit = get_revision(&rev)) != NULL) > - find_unpushed_submodule_commits(commit, needs_pushing); > + find_unpushed_submodule_commits(commit, &submodules); > > reset_revision_walk(); > free(sha1_copy); > strbuf_release(&remotes_arg); > > + for
Re: "Purposes, Concepts,Misfits, and a Redesign of Git" (a research paper)
On 7 October 2016 at 18:55, Santiago Perez De Rosso wrote: > On Wed, Oct 5, 2016 at 6:15 AM Jakub Narębski wrote: >> On 5 October 2016 at 04:55, Santiago Perez De Rosso >> wrote: >>> On Fri, Sep 30, 2016 at 6:25 PM Jakub Narębski wrote: W dniu 30.09.2016 o 18:14, Konstantin Khomoutov pisze: > The "It Will Never Work in Theory" blog has just posted a summary of a > study which tried to identify shortcomings in the design of Git. > > In the hope it might be interesting, I post this summary here. > URL: http://neverworkintheory.org/2016/09/30/rethinking-git.html I will comment on the article itself, not just on the summary. | 2.2 Git [...] | But tracked files cannot be ignored; to ignore a tracked file | one has to mark it as “assume unchanged.” This “assume | unchanged” file will not be recognized by add; to make it | tracked again this marking has to be removed. [...] >> Yes, this is true that users may want to be able to ignore changes to >> tracked files (commit with dirty tree), but using `assume-unchanged` is >> wrong and dangerous solution. Unfortunately the advice to use it is >> surprisingly pervasive. I would thank you to not further this error. > > Ok, I added a footnote in the paper when we first mention assume unchanged > that says: > > Assume unchanged was intended to be used as a performance optimization but > has since been appropriated by users as a way to ignore tracked files. The > current advice is to use the “skip worktree” marking instead > > This should prompt readers to look into skip worktree next time they want to > ignore tracked files. I don't think people reading the paper are doing so to > learn Git but at least it should contribute to not furthering the error. Thank you very much. The problem with "assume-unchanged" is that by using it to ignore changes to tracked files you are lying to Git (telling it 'assume this is unchanged' while changing it), and can lead to DATA LOSS, that is to losing those changes. [...] [...] | The problem | is the lack of connection between this purpose and the highlevel | purposes for version control, which suggests that the | introduction of stashing might be to patch flaws in the design | of Git and not to satisfy a requirement of version control. Or the problem might be that you are missing some (maybe minor) requirement of version control system. Just saying... >>> >>> What would that purpose be? and why would you say that's a >>> high-level purpose for version control and not one that's >>> git-specific? >> >> The stash (or rather its equivalent) is not something Git specific. >> It is present also in other version control systems, among others: >> >> * Mercurial: as 'shelve' extension (in core since 1.8) >> * Bazaar: as 'bzr shelve' command >> * Fossil: as 'fossil stash' command (with subcommands) >> * Subversion: Shelve planned for 1.10 (2017?) > > Do these other VCSs have the same "Switching branches" misfit? Do you > usually need to stash if you want to switch with uncommitted changes? I know > Mercurial has the same problem since ``bookmarks'' are like Git branches, so > it makes sense for them to have added something like stashing (if otherwise > switching with uncommitted changes would be very difficult). I suspect that all those are inspired by each other, and that they all use 'uncommitted changes are not tied to a branch' paradigm, which allows for creating a branch for changes after a fact (when it turns out that it would take more than one commit to implement the feature) quite easy. >> I would say that 'stash' could be considered about isolating work on >> different features, different sub-branch sized parallel work. Note that 'isolating work' is missing from your list of purposes of a version control system; though it is fairly obvious. > > That sounds a lot like having independent lines of development, which is > what branches are supposed to be for Those are sub-commit changes. Branches are composed of commits. But I agree that this may be a bit of a stretch in trying to find a high-level purpose for stash (rather than it being a convenience feature). As I said below... >> But it might be that stash doesn't have connection with highlevel >> purposes for version control, and that it is purely convenience >> feature. Just playing the role of Advocatus Diaboli (important in >> scientific works, isn' it?)... [...] | 7. Gitless [...] | Also, there | is no possible way of getting in a “detached head” state; at | any time, the user is always working on some branch (the | “current” branch). Head is a per-branch reference to the last | commit of the branch. [...] [...] during some long lived multi-step operations, like bisect or interactive rebase, you are not really on any branch, >>> >>> In Gitless we don't have bisect but for rebase (fuse in Gitless) we >>> re
Re: "Purposes, Concepts,Misfits, and a Redesign of Git" (a research paper)
On Fri, 30 Sep 2016 19:14:13 +0300 Konstantin Khomoutov wrote: > The "It Will Never Work in Theory" blog has just posted a summary of a > study which tried to identify shortcomings in the design of Git. > > In the hope it might be interesting, I post this summary here. > URL: http://neverworkintheory.org/2016/09/30/rethinking-git.html I think it would be cool if someone among subscribers could post a link to our discussion thread [2] back onto that page. Unfortunatrly that requires a Disqus login which I don't have, so may be someone in possession of such login could do that? ;-) 2. https://public-inbox.org/git/ce42f934-4a94-fa29-cff0-5ebb0f004...@gmail.com/T/#e95875b7940512b432ab2e29b3dd50ca448df9720
Re: What's cooking in git.git (Oct 2016, #02; Thu, 6)
On Fri, Oct 07, 2016 at 09:40:01AM -0700, Junio C Hamano wrote: > Kevin Daudt writes: > > > Just wondering why the topic "kd/mailinfo-quoted-string (2016-09-28) 2 > > commits" is not listed anymore. Previous what's cooking said it was > > merged to "next". > > Thanks for your contribution. > > The topic was listed as graduated to 'master' in issue #1 of this > month: > > http://public-inbox.org/git/ > > After a topic graduates, it will not be included in the report. Ok, was confused then about what the state was. Thanks.
Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
On Fri, Oct 07, 2016 at 07:42:35PM +0200, Johannes Sixt wrote: > Maybe it's time to aim for > > git config alias.d2u.shell \ >'f() { git ls-files "$@" | xargs dos2unix; }; f' > git config alias.d2u.cdup false > git d2u *.c # yada! That would be nice. It would also allow "alias.foo_bar.shell"; right now "alias.foo_bar" is forbidden because of the config syntax, which does not allow underscores outside of the "subsection" name. -Peff
Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
Am 07.10.2016 um 14:27 schrieb Duy Nguyen: On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin wrote: Hi Junio, On Thu, 6 Oct 2016, Junio C Hamano wrote: Nguyễn Thái Ngọc Duy writes: Throwing something at the mailing list to see if anybody is interested. Current '!' aliases move cwd to $GIT_WORK_TREE first, which could make handling path arguments hard because they are relative to the original cwd. We set GIT_PREFIX to work around it, but I still think it's more natural to keep cwd where it is. We have a way to do that now after 441981b (git: simplify environment save/restore logic - 2016-01-26). It's just a matter of choosing the right syntax. I'm going with '!!'. I'm not very happy with it. But I do like this type of alias. I do not know why you are not happy with the syntax, but I personally think it brilliant, both the idea and the preliminary clean-up that made this possible with a simple patch like this. I guess he is not happy with it because "!!" is quite unintuitive a construct. I know that *I* would have been puzzled by it, asking "What the heck does this do?". Yep. And I wouldn't want to set a tradition for the next alias type '!!!'. There's no good choice to represent a new alias type with a leading symbol. This just occurred to me, however, what do you think about a new config group for it? With can have something like externalAlias.* (or some other name) that lives in parallel with alias.*. Then we don't need '!' (or '!!') at all. Maybe it's time to aim for git config alias.d2u.shell \ 'f() { git ls-files "$@" | xargs dos2unix; }; f' git config alias.d2u.cdup false git d2u *.c # yada! -- Hannes
Re: [PATCH 1/2] submodule add: extend force flag to add existing repos
On Fri, Oct 7, 2016 at 5:52 AM, Heiko Voigt wrote: > On Thu, Oct 06, 2016 at 01:11:20PM -0700, Junio C Hamano wrote: >> Stefan Beller writes: >> >> > Currently the force flag in `git submodule add` takes care of possibly >> > ignored files or when a name collision occurs. >> > >> > However there is another situation where submodule add comes in handy: >> > When you already have a gitlink recorded, but no configuration was >> > done (i.e. no .gitmodules file nor any entry in .git/config) and you >> > want to generate these config entries. For this situation allow >> > `git submodule add` to proceed if there is already a submodule at the >> > given path in the index. > > Is it important that the submodule is in the index? If it is not in the index, it already works. > How about worktree? > From the index entry alone we can not deduce the values anyway. Right, but as of now this is the only show stopper, i.e. * you have an existing repo? -> fine, it works with --force * you even ignored that repo -> --force knows how to do it. * you already have a gitlink -> Sorry, you're out of luck. So that is why I stressed index in this commit message, as it is only about this case. > > [1] http://public-inbox.org/git/%3c20160916141143.ga47...@book.hvoigt.net%3E/ Current situation: > clone the submodule into a directory > git submodule add --force > git commit everything works fine, but: > clone the submodule into a directory > git add > git commit -m "Add submodule" > # me: "Oh crap! I did forget the configuration." > git submodule add --force > # Git: "It already exists in the index, I am not going to produce the config > for you." The last step is changed with this patch, as it will just work fine then.
Re: [PATCH 4/4] mergetool: honor -O
Am 07.10.2016 um 01:47 schrieb David Aguilar: diff --git a/git-mergetool.sh b/git-mergetool.sh index 65696d8..9dca58b 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -9,7 +9,7 @@ # at the discretion of Junio C Hamano. # -USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...' +USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-O] [file to merge] ...' SUBDIRECTORY_OK=Yes NONGIT_OK=Yes OPTIONS_SPEC= @@ -390,6 +390,7 @@ print_noop_and_exit () { main () { prompt=$(git config --bool mergetool.prompt) guessed_merge_tool=false + order_file= while test $# != 0 do @@ -419,6 +420,9 @@ main () { --prompt) prompt=true ;; + -O*) + order_file="$1" + ;; --) shift break @@ -459,8 +463,14 @@ main () { fi fi + if test -n "$order_file" + then + set -- "$order_file" -- "$@" + else + set -- -- "$@" + fi files=$(git -c core.quotePath=false \ - diff --name-only --diff-filter=U -- "$@") + diff --name-only --diff-filter=U "$@") You can write this as files=$(git -c core.quotePath=false \ diff --name-only --diff-filter=U \ ${order_file:+"$order_file"} -- "$@") without the case distinction earlier. This construct is usually used to attach the option (-O) in front of the argument, but it is already included in the value; so, we use the construct only to avoid an empty argument when the option is unset and to get a quoted string when it is set. cd_to_toplevel diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh index fb2aeef..0f9869a 100755 --- a/t/t7610-mergetool.sh +++ b/t/t7610-mergetool.sh @@ -637,6 +637,33 @@ test_expect_success 'diff.orderFile configuration is honored' ' test_cmp expect actual && git reset --hard >/dev/null ' + +test_expect_success 'mergetool -Oorder-file is honored' ' + test_config diff.orderFile order-file && + test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" && + test_config mergetool.myecho.trustExitCode true && + test_must_fail git merge order-file-side1 && + cat >expect <<-\EOF && + Merging: + a + b + EOF + git mergetool -O/dev/null --no-prompt --tool myecho | + grep -A 2 Merging: >actual && + test_cmp expect actual && + git reset --hard >/dev/null 2>&1 && + + git config --unset diff.orderFile && + test_must_fail git merge order-file-side1 && + cat >expect <<-\EOF && + Merging: + b + a + EOF + git mergetool -Oorder-file --no-prompt --tool myecho | + grep -A 2 Merging: >actual && grep -A again. Furthermore, you call git mergetool in the upstream of a pipe. This will ignore the exit status. + test_cmp expect actual && + git reset --hard >/dev/null 2>&1 ' Ah, the earlier lone quote should have been added in this patch. -- Hannes
Re: [PATCH 3/4] mergetool: honor diff.orderFile
Am 07.10.2016 um 01:47 schrieb David Aguilar: @@ -606,4 +606,37 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT git reset --hard master >/dev/null 2>&1 ' +test_expect_success 'diff.orderFile configuration is honored' ' + test_config diff.orderFile order-file && + test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" && + test_config mergetool.myecho.trustExitCode true && + echo b >order-file && + echo a >>order-file && + git checkout -b order-file-start master && + echo start >a && + echo start >b && + git add a b && + git commit -m start && + git checkout -b order-file-side1 order-file-start && + echo side1 >a && + echo side1 >b && + git add a b && + git commit -m side1 && + git checkout -b order-file-side2 order-file-start && + echo side2 >a && + echo side2 >b && + git add a b && + git commit -m side2 && + test_must_fail git merge order-file-side1 && + cat >expect <<-\EOF && + Merging: + b + a + EOF + git mergetool --no-prompt --tool myecho | grep -A 2 Merging: >actual && Is grep -A universally available? + test_cmp expect actual && + git reset --hard >/dev/null +' +' Two single-quotes? + test_done Otherwise the patch looks good. -- Hannes
[PATCH v2] l10n: de.po: translate 260 new messages
Translate 260 new message came from git.pot updates in 9fa976f (l10n: git.pot: v2.10.0 round 1 (248 new, 56 removed)) and 5bd166d (l10n: git.pot: v2.10.0 round 2 (12 new, 44 removed)). Signed-off-by: Ralf Thielow --- po/de.po | 783 ++- 1 file changed, 421 insertions(+), 362 deletions(-) diff --git a/po/de.po b/po/de.po index e1865c6ca..0755cdf6c 100644 --- a/po/de.po +++ b/po/de.po @@ -14,72 +14,60 @@ msgstr "" "Language: de\n" "MIME-Version: 1.0\n" "Content-Type: text/plain; charset=UTF-8\n" "Content-Transfer-Encoding: 8bit\n" "Plural-Forms: nplurals=2; plural=(n!=1);\n" #: advice.c:55 #, c-format msgid "hint: %.*s\n" msgstr "Hinweis: %.*s\n" #: advice.c:83 -#, fuzzy msgid "Cherry-picking is not possible because you have unmerged files." -msgstr "" -"\"pull\" ist nicht möglich, weil Sie nicht zusammengeführte Dateien haben." +msgstr "Cherry-Picken ist nicht möglich, weil Sie nicht zusammengeführte Dateien haben." #: advice.c:85 -#, fuzzy msgid "Committing is not possible because you have unmerged files." -msgstr "" -"\"pull\" ist nicht möglich, weil Sie nicht zusammengeführte Dateien haben." +msgstr "Committen ist nicht möglich, weil Sie nicht zusammengeführte Dateien haben." #: advice.c:87 -#, fuzzy msgid "Merging is not possible because you have unmerged files." -msgstr "" -"\"pull\" ist nicht möglich, weil Sie nicht zusammengeführte Dateien haben." +msgstr "Mergen ist nicht möglich, weil Sie nicht zusammengeführte Dateien haben." #: advice.c:89 -#, fuzzy msgid "Pulling is not possible because you have unmerged files." -msgstr "" -"\"pull\" ist nicht möglich, weil Sie nicht zusammengeführte Dateien haben." +msgstr "Pullen ist nicht möglich, weil Sie nicht zusammengeführte Dateien haben." #: advice.c:91 -#, fuzzy msgid "Reverting is not possible because you have unmerged files." -msgstr "" -"\"pull\" ist nicht möglich, weil Sie nicht zusammengeführte Dateien haben." +msgstr "Reverten ist nicht möglich, weil Sie nicht zusammengeführte Dateien haben." #: advice.c:93 -#, fuzzy, c-format +#, c-format msgid "It is not possible to %s because you have unmerged files." -msgstr "" -"\"pull\" ist nicht möglich, weil Sie nicht zusammengeführte Dateien haben." +msgstr "%s ist nicht möglich, weil Sie nicht zusammengeführte Dateien haben." #: advice.c:101 msgid "" "Fix them up in the work tree, and then use 'git add/rm '\n" "as appropriate to mark resolution and make a commit." msgstr "" "Korrigieren Sie dies im Arbeitsverzeichnis, und benutzen Sie\n" "dann 'git add/rm ', um die Auflösung entsprechend zu markieren\n" "und zu committen." #: advice.c:109 -#, fuzzy msgid "Exiting because of an unresolved conflict." -msgstr "Beende wegen nicht abgeschlossenem Merge." +msgstr "Beende wegen unaufgelöstem Konflikt." #: advice.c:114 builtin/merge.c:1181 msgid "You have not concluded your merge (MERGE_HEAD exists)." msgstr "Sie haben Ihren Merge nicht abgeschlossen (MERGE_HEAD existiert)." #: advice.c:116 msgid "Please, commit your changes before merging." msgstr "Bitte committen Sie Ihre Änderungen, bevor Sie mergen." #: advice.c:117 msgid "Exiting because of unfinished merge." msgstr "Beende wegen nicht abgeschlossenem Merge." @@ -90,24 +78,38 @@ msgid "" "Note: checking out '%s'.\n" "\n" "You are in 'detached HEAD' state. You can look around, make experimental\n" "changes and commit them, and you can discard any commits you make in this\n" "state without impacting any branches by performing another checkout.\n" "\n" "If you want to create a new branch to retain commits you create, you may\n" "do so (now or later) by using -b with the checkout command again. Example:\n" "\n" " git checkout -b \n" "\n" msgstr "" +"Hinweis: Checke '%s' aus.\n" +"\n" +"Sie befinden sich im Zustand eines 'lösgelösten HEAD'. Sie können sich\n" +"umschauen, experimentelle Änderungen vornehmen und diese committen, und\n" +"Sie können alle möglichen Commits, die Sie in diesem Zustand machen,\n" +"ohne Auswirkungen auf irgendeinen Branch verwerfen, indem Sie einen\n" +"weiteren Checkout durchführen.\n" +"\n" +"Wenn Sie einen neuen Branch erstellen möchten, um Ihre erstellten Commits\n" +"zu behalten, können Sie das (jetzt oder später) durch einen weiteren Checkout\n" +"mit der Option -b tun. Beispiel:\n" +"\n" +" git checkout -b \n" +"\n" #: archive.c:12 msgid "git archive [] [...]" msgstr "git archive [] [...]" #: archive.c:13 msgid "git archive --list" msgstr "git archive --list" #: archive.c:14 msgid "" "git archive --remote [--exec ] [] [...]" @@ -185,165 +187,176 @@ msgstr "Repository" msgid "retrieve the archive from remote repository " msgstr "Archiv vom Remote-Repository abrufen" #: archive.c:453 builtin/archive.c:92 builtin/notes.c:483 msgid "command" msgstr "Programm" #: archive.c:454 builtin/archive.c:93 msgid "path to the remote git-upload-archive command" msgs
Re: What's cooking in git.git (Oct 2016, #02; Thu, 6)
Kevin Daudt writes: > Just wondering why the topic "kd/mailinfo-quoted-string (2016-09-28) 2 > commits" is not listed anymore. Previous what's cooking said it was > merged to "next". Thanks for your contribution. The topic was listed as graduated to 'master' in issue #1 of this month: http://public-inbox.org/git/ After a topic graduates, it will not be included in the report.
Re: [PATCH v4 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c
W dniu 07.10.2016 o 18:08, Johannes Schindelin pisze: > - marked the hint "please commit or stash them" (reintroduced from the > original git-pull.sh script) as translatable. I wonder if we can make automatic check if everything introduced is translatable, for example with something akin to "English (XT)" autogenerated pseudo-localization that Android uses? Just food for thought. -- Jakub Narębski
Re: [PATCH v4 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c
Johannes Schindelin writes: > - changed the exit code to 128 (emulating a die()) if > require_clean_work-tree() was asked to be non-gentle. Ah, I didn't spot this the last round. Good change. Thanks, will replace.
Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
W dniu 07.10.2016 o 16:19, Johannes Schindelin pisze: > On Fri, 7 Oct 2016, Jakub Narębski wrote: >> Note that we would have to teach git completion about new syntax; >> or new configuration variable if we go that route. > > Why would we? Git's completion does not expand aliases, it only completes > the aliases' names, not their values. Because Git completion finds out which _options_ and _arguments_ to complete for an alias based on its expansion. Yes, this is nice bit of trickery... -- Jakub Narębski
[PATCH v4 4/6] wt-status: export also the has_un{staged,committed}_changes() functions
They will be used in the upcoming rebase helper. Signed-off-by: Johannes Schindelin --- wt-status.c | 4 ++-- wt-status.h | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/wt-status.c b/wt-status.c index 89475f1..8fe7d0a 100644 --- a/wt-status.c +++ b/wt-status.c @@ -2213,7 +2213,7 @@ void wt_status_print(struct wt_status *s) /** * Returns 1 if there are unstaged changes, 0 otherwise. */ -static int has_unstaged_changes(void) +int has_unstaged_changes(void) { struct rev_info rev_info; int result; @@ -2229,7 +2229,7 @@ static int has_unstaged_changes(void) /** * Returns 1 if there are uncommitted changes, 0 otherwise. */ -static int has_uncommitted_changes(void) +int has_uncommitted_changes(void) { struct rev_info rev_info; int result; diff --git a/wt-status.h b/wt-status.h index 68b4709..68e367a 100644 --- a/wt-status.h +++ b/wt-status.h @@ -128,7 +128,9 @@ void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, . __attribute__((format (printf, 3, 4))) void status_printf(struct wt_status *s, const char *color, const char *fmt, ...); -/* The following function expects that the caller took care of reading the index. */ +/* The following functions expect that the caller took care of reading the index. */ +int has_unstaged_changes(void); +int has_uncommitted_changes(void); int require_clean_work_tree(const char *action, const char *hint, int gently); #endif /* STATUS_H */ -- 2.10.0.windows.1.325.ge6089c1
[PATCH v4 1/6] pull: drop confusing prefix parameter of die_on_unclean_work_tree()
In cmd_pull(), when verifying that there are no changes preventing a rebasing pull, we diligently pass the prefix parameter to the die_on_unclean_work_tree() function which in turn diligently passes it to the has_unstaged_changes() and has_uncommitted_changes() functions. The casual reader might now be curious (as this developer was) whether that means that calling `git pull --rebase` in a subdirectory will ignore unstaged changes in other parts of the working directory. And be puzzled that `git pull --rebase` (correctly) complains about those changes outside of the current directory. The puzzle is easily resolved: while we take pains to pass around the prefix and even pass it to init_revisions(), the fact that no paths are passed to init_revisions() ensures that the prefix is simply ignored. That, combined with the fact that we will *always* want a *full* working directory check before running a rebasing pull, is reason enough to simply do away with the actual prefix parameter and to pass NULL instead, as if we were running this from the top-level working directory anyway. Signed-off-by: Johannes Schindelin --- builtin/pull.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 398aae1..d4bd635 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -328,12 +328,12 @@ static int git_pull_config(const char *var, const char *value, void *cb) /** * Returns 1 if there are unstaged changes, 0 otherwise. */ -static int has_unstaged_changes(const char *prefix) +static int has_unstaged_changes(void) { struct rev_info rev_info; int result; - init_revisions(&rev_info, prefix); + init_revisions(&rev_info, NULL); DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES); DIFF_OPT_SET(&rev_info.diffopt, QUICK); diff_setup_done(&rev_info.diffopt); @@ -344,7 +344,7 @@ static int has_unstaged_changes(const char *prefix) /** * Returns 1 if there are uncommitted changes, 0 otherwise. */ -static int has_uncommitted_changes(const char *prefix) +static int has_uncommitted_changes(void) { struct rev_info rev_info; int result; @@ -352,7 +352,7 @@ static int has_uncommitted_changes(const char *prefix) if (is_cache_unborn()) return 0; - init_revisions(&rev_info, prefix); + init_revisions(&rev_info, NULL); DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES); DIFF_OPT_SET(&rev_info.diffopt, QUICK); add_head_to_pending(&rev_info); @@ -365,7 +365,7 @@ static int has_uncommitted_changes(const char *prefix) * If the work tree has unstaged or uncommitted changes, dies with the * appropriate message. */ -static void die_on_unclean_work_tree(const char *prefix) +static void die_on_unclean_work_tree(void) { struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file)); int do_die = 0; @@ -375,12 +375,12 @@ static void die_on_unclean_work_tree(const char *prefix) update_index_if_able(&the_index, lock_file); rollback_lock_file(lock_file); - if (has_unstaged_changes(prefix)) { + if (has_unstaged_changes()) { error(_("Cannot pull with rebase: You have unstaged changes.")); do_die = 1; } - if (has_uncommitted_changes(prefix)) { + if (has_uncommitted_changes()) { if (do_die) error(_("Additionally, your index contains uncommitted changes.")); else @@ -875,7 +875,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) die(_("Updating an unborn branch with changes added to the index.")); if (!autostash) - die_on_unclean_work_tree(prefix); + die_on_unclean_work_tree(); if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs)) hashclr(rebase_fork_point); -- 2.10.0.windows.1.325.ge6089c1
[PATCH v4 3/6] wt-status: make the require_clean_work_tree() function reusable
The function used by "git pull" to stop the user when the working tree has changes is useful in other places. Let's move it into a more prominent (and into an actually reusable) spot: wt-status.[ch]. Signed-off-by: Johannes Schindelin --- builtin/pull.c | 77 +- wt-status.c| 76 + wt-status.h| 3 +++ 3 files changed, 80 insertions(+), 76 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 58fc176..01b6465 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -17,6 +17,7 @@ #include "revision.h" #include "tempfile.h" #include "lockfile.h" +#include "wt-status.h" enum rebase_type { REBASE_INVALID = -1, @@ -326,82 +327,6 @@ static int git_pull_config(const char *var, const char *value, void *cb) } /** - * Returns 1 if there are unstaged changes, 0 otherwise. - */ -static int has_unstaged_changes(void) -{ - struct rev_info rev_info; - int result; - - init_revisions(&rev_info, NULL); - DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES); - DIFF_OPT_SET(&rev_info.diffopt, QUICK); - diff_setup_done(&rev_info.diffopt); - result = run_diff_files(&rev_info, 0); - return diff_result_code(&rev_info.diffopt, result); -} - -/** - * Returns 1 if there are uncommitted changes, 0 otherwise. - */ -static int has_uncommitted_changes(void) -{ - struct rev_info rev_info; - int result; - - if (is_cache_unborn()) - return 0; - - init_revisions(&rev_info, NULL); - DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES); - DIFF_OPT_SET(&rev_info.diffopt, QUICK); - add_head_to_pending(&rev_info); - diff_setup_done(&rev_info.diffopt); - result = run_diff_index(&rev_info, 1); - return diff_result_code(&rev_info.diffopt, result); -} - -/** - * If the work tree has unstaged or uncommitted changes, dies with the - * appropriate message. - */ -static int require_clean_work_tree(const char *action, const char *hint, - int gently) -{ - struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file)); - int err = 0; - - hold_locked_index(lock_file, 0); - refresh_cache(REFRESH_QUIET); - update_index_if_able(&the_index, lock_file); - rollback_lock_file(lock_file); - - if (has_unstaged_changes()) { - /* TRANSLATORS: the action is e.g. "pull with rebase" */ - error(_("Cannot %s: You have unstaged changes."), _(action)); - err = 1; - } - - if (has_uncommitted_changes()) { - if (err) - error(_("Additionally, your index contains uncommitted changes.")); - else - error(_("Cannot %s: Your index contains uncommitted changes."), - _(action)); - err = 1; - } - - if (err) { - if (hint) - error("%s", hint); - if (!gently) - exit(128); - } - - return err; -} - -/** * Appends merge candidates from FETCH_HEAD that are not marked not-for-merge * into merge_heads. */ diff --git a/wt-status.c b/wt-status.c index 99d1b0a..89475f1 100644 --- a/wt-status.c +++ b/wt-status.c @@ -16,6 +16,7 @@ #include "strbuf.h" #include "utf8.h" #include "worktree.h" +#include "lockfile.h" static const char cut_line[] = " >8 \n"; @@ -2208,3 +2209,78 @@ void wt_status_print(struct wt_status *s) break; } } + +/** + * Returns 1 if there are unstaged changes, 0 otherwise. + */ +static int has_unstaged_changes(void) +{ + struct rev_info rev_info; + int result; + + init_revisions(&rev_info, NULL); + DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES); + DIFF_OPT_SET(&rev_info.diffopt, QUICK); + diff_setup_done(&rev_info.diffopt); + result = run_diff_files(&rev_info, 0); + return diff_result_code(&rev_info.diffopt, result); +} + +/** + * Returns 1 if there are uncommitted changes, 0 otherwise. + */ +static int has_uncommitted_changes(void) +{ + struct rev_info rev_info; + int result; + + if (is_cache_unborn()) + return 0; + + init_revisions(&rev_info, NULL); + DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES); + DIFF_OPT_SET(&rev_info.diffopt, QUICK); + add_head_to_pending(&rev_info); + diff_setup_done(&rev_info.diffopt); + result = run_diff_index(&rev_info, 1); + return diff_result_code(&rev_info.diffopt, result); +} + +/** + * If the work tree has unstaged or uncommitted changes, dies with the + * appropriate message. + */ +int require_clean_work_tree(const char *action, const char *hint, int gently) +{ + struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file)); + int err = 0; + +
[PATCH v4 2/6] pull: make code more similar to the shell script again
When converting the pull command to a builtin, the require_clean_work_tree() function was renamed and the pull-specific parts hard-coded. This makes it impossible to reuse the code, so let's modify the code to make it more similar to the original shell script again. Note: when the hint "Please commit or stash them" was introduced first, Git did not have the convention of continuing error messages in lower case, but now we do have that convention, therefore we reintroduce this hint down-cased, obeying said convention. Signed-off-by: Johannes Schindelin --- builtin/pull.c | 30 -- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index d4bd635..58fc176 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -365,10 +365,11 @@ static int has_uncommitted_changes(void) * If the work tree has unstaged or uncommitted changes, dies with the * appropriate message. */ -static void die_on_unclean_work_tree(void) +static int require_clean_work_tree(const char *action, const char *hint, + int gently) { struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file)); - int do_die = 0; + int err = 0; hold_locked_index(lock_file, 0); refresh_cache(REFRESH_QUIET); @@ -376,20 +377,28 @@ static void die_on_unclean_work_tree(void) rollback_lock_file(lock_file); if (has_unstaged_changes()) { - error(_("Cannot pull with rebase: You have unstaged changes.")); - do_die = 1; + /* TRANSLATORS: the action is e.g. "pull with rebase" */ + error(_("Cannot %s: You have unstaged changes."), _(action)); + err = 1; } if (has_uncommitted_changes()) { - if (do_die) + if (err) error(_("Additionally, your index contains uncommitted changes.")); else - error(_("Cannot pull with rebase: Your index contains uncommitted changes.")); - do_die = 1; + error(_("Cannot %s: Your index contains uncommitted changes."), + _(action)); + err = 1; } - if (do_die) - exit(1); + if (err) { + if (hint) + error("%s", hint); + if (!gently) + exit(128); + } + + return err; } /** @@ -875,7 +884,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix) die(_("Updating an unborn branch with changes added to the index.")); if (!autostash) - die_on_unclean_work_tree(); + require_clean_work_tree(N_("pull with rebase"), + _("please commit or stash them."), 0); if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs)) hashclr(rebase_fork_point); -- 2.10.0.windows.1.325.ge6089c1
[PATCH v4 5/6] wt-status: teach has_{unstaged,uncommitted}_changes() about submodules
Sometimes we are *actually* interested in those changes... For example when an interactive rebase wants to continue with a staged submodule update. Signed-off-by: Johannes Schindelin --- builtin/pull.c | 2 +- wt-status.c| 16 +--- wt-status.h| 7 --- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 01b6465..d6e46ee 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (!autostash) require_clean_work_tree(N_("pull with rebase"), - _("please commit or stash them."), 0); + _("please commit or stash them."), 1, 0); if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs)) hashclr(rebase_fork_point); diff --git a/wt-status.c b/wt-status.c index 8fe7d0a..0020140 100644 --- a/wt-status.c +++ b/wt-status.c @@ -2213,13 +2213,14 @@ void wt_status_print(struct wt_status *s) /** * Returns 1 if there are unstaged changes, 0 otherwise. */ -int has_unstaged_changes(void) +int has_unstaged_changes(int ignore_submodules) { struct rev_info rev_info; int result; init_revisions(&rev_info, NULL); - DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES); + if (ignore_submodules) + DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES); DIFF_OPT_SET(&rev_info.diffopt, QUICK); diff_setup_done(&rev_info.diffopt); result = run_diff_files(&rev_info, 0); @@ -2229,7 +2230,7 @@ int has_unstaged_changes(void) /** * Returns 1 if there are uncommitted changes, 0 otherwise. */ -int has_uncommitted_changes(void) +int has_uncommitted_changes(int ignore_submodules) { struct rev_info rev_info; int result; @@ -2238,7 +2239,8 @@ int has_uncommitted_changes(void) return 0; init_revisions(&rev_info, NULL); - DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES); + if (ignore_submodules) + DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES); DIFF_OPT_SET(&rev_info.diffopt, QUICK); add_head_to_pending(&rev_info); diff_setup_done(&rev_info.diffopt); @@ -2250,7 +2252,7 @@ int has_uncommitted_changes(void) * If the work tree has unstaged or uncommitted changes, dies with the * appropriate message. */ -int require_clean_work_tree(const char *action, const char *hint, int gently) +int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently) { struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file)); int err = 0; @@ -2260,13 +2262,13 @@ int require_clean_work_tree(const char *action, const char *hint, int gently) update_index_if_able(&the_index, lock_file); rollback_lock_file(lock_file); - if (has_unstaged_changes()) { + if (has_unstaged_changes(ignore_submodules)) { /* TRANSLATORS: the action is e.g. "pull with rebase" */ error(_("Cannot %s: You have unstaged changes."), _(action)); err = 1; } - if (has_uncommitted_changes()) { + if (has_uncommitted_changes(ignore_submodules)) { if (err) error(_("Additionally, your index contains uncommitted changes.")); else diff --git a/wt-status.h b/wt-status.h index 68e367a..54fec77 100644 --- a/wt-status.h +++ b/wt-status.h @@ -129,8 +129,9 @@ __attribute__((format (printf, 3, 4))) void status_printf(struct wt_status *s, const char *color, const char *fmt, ...); /* The following functions expect that the caller took care of reading the index. */ -int has_unstaged_changes(void); -int has_uncommitted_changes(void); -int require_clean_work_tree(const char *action, const char *hint, int gently); +int has_unstaged_changes(int ignore_submodules); +int has_uncommitted_changes(int ignore_submodules); +int require_clean_work_tree(const char *action, const char *hint, + int ignore_submodules, int gently); #endif /* STATUS_H */ -- 2.10.0.windows.1.325.ge6089c1
[PATCH v4 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c
This is the 5th last patch series of my work to accelerate interactive rebases in particular on Windows. Basically, all it does is to make reusable some functions that were ported over from git-pull.sh but made private to builtin/pull.c. Changes since v3: - reworded 3/5's commit message according to Junio's suggestion. - fixed a tyop in 4/5's commit message, pointed out by Jakub. - marked the hint "please commit or stash them" (reintroduced from the original git-pull.sh script) as translatable. - changed the exit code to 128 (emulating a die()) if require_clean_work-tree() was asked to be non-gentle. - fixed a tyop in 3/6 (which was replaced in 4/6, but it is good not to introduce bugs only to fix them right away). - prefixed the commit message of 4/6 with the "wt-status:" prefix, replicating Junio's commit message in the `pu` branch. Johannes Schindelin (6): pull: drop confusing prefix parameter of die_on_unclean_work_tree() pull: make code more similar to the shell script again wt-status: make the require_clean_work_tree() function reusable wt-status: export also the has_un{staged,committed}_changes() functions wt-status: teach has_{unstaged,uncommitted}_changes() about submodules wt-status: begin error messages with lower-case builtin/pull.c | 71 +++- wt-status.c| 78 ++ wt-status.h| 6 + 3 files changed, 87 insertions(+), 68 deletions(-) Published-As: https://github.com/dscho/git/releases/tag/require-clean-work-tree-v4 Fetch-It-Via: git fetch https://github.com/dscho/git require-clean-work-tree-v4 Interdiff vs v3: diff --git a/builtin/pull.c b/builtin/pull.c index 0bf9802..d6e46ee 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (!autostash) require_clean_work_tree(N_("pull with rebase"), - "please commit or stash them.", 1, 0); + _("please commit or stash them."), 1, 0); if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs)) hashclr(rebase_fork_point); diff --git a/wt-status.c b/wt-status.c index ef67593..e8e5de4 100644 --- a/wt-status.c +++ b/wt-status.c @@ -2281,7 +2281,7 @@ int require_clean_work_tree(const char *action, const char *hint, int ignore_sub if (hint) error("%s", hint); if (!gently) - exit(err); + exit(128); } return err; -- 2.10.0.windows.1.325.ge6089c1 base-commit: a23ca1b8dc42ffd4de2ef30d67ce1e21ded29886
[PATCH v4 6/6] wt-status: begin error messages with lower-case
The previous code still followed the old git-pull.sh code which did not adhere to our new convention. Signed-off-by: Johannes Schindelin --- wt-status.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/wt-status.c b/wt-status.c index 0020140..e8e5de4 100644 --- a/wt-status.c +++ b/wt-status.c @@ -2264,15 +2264,15 @@ int require_clean_work_tree(const char *action, const char *hint, int ignore_sub if (has_unstaged_changes(ignore_submodules)) { /* TRANSLATORS: the action is e.g. "pull with rebase" */ - error(_("Cannot %s: You have unstaged changes."), _(action)); + error(_("cannot %s: You have unstaged changes."), _(action)); err = 1; } if (has_uncommitted_changes(ignore_submodules)) { if (err) - error(_("Additionally, your index contains uncommitted changes.")); + error(_("additionally, your index contains uncommitted changes.")); else - error(_("Cannot %s: Your index contains uncommitted changes."), + error(_("cannot %s: Your index contains uncommitted changes."), _(action)); err = 1; } -- 2.10.0.windows.1.325.ge6089c1
Re: Systems with old regex system headers/libraries don't pick up git's compat/regex header file
On Fri, Oct 07, 2016 at 04:45:08PM +0100, Richard Lloyd wrote: > On 06/10/16 20:11, Jeff King wrote: > > Junio mentioned the NO_REGEX knob in the Makefile. If that works for > > you, the next step is probably to add a line to the HP-UX section of > > config.mak.uname, so that it just works out of the box. > > This doesn't work because the check in git-compat-util.h only looks > for REG_STARTEND being defined (if it isn't, it #error's out). > > That define is not mentioned anywhere else other than in the > compat/regex tree, which is why I used -Icompat/regex to pick up > from there - this was the "easiest" solution for me on > HP-UX 11. I'm confused. Setting NO_REGEX in the Makefile will add -Icompat/regex to your compiler invocation. So git-compat-util.h should pick up our compat regex routines, which _do_ have REG_STARTEND. How are you building? Doing: make NO_REGEX=1 is supposed to work, and if it doesn't, there's a bug. -Peff
Re: Systems with old regex system headers/libraries don't pick up git's compat/regex header file
On 06/10/16 20:11, Jeff King wrote: Junio mentioned the NO_REGEX knob in the Makefile. If that works for you, the next step is probably to add a line to the HP-UX section of config.mak.uname, so that it just works out of the box. This doesn't work because the check in git-compat-util.h only looks for REG_STARTEND being defined (if it isn't, it #error's out). That define is not mentioned anywhere else other than in the compat/regex tree, which is why I used -Icompat/regex to pick up from there - this was the "easiest" solution for me on HP-UX 11. Note that with this inclusion change, the source compiled and linked fine on HP-UX 11 and git passed its tests, including the regex-based ones. Richard K. Lloyd, E-mail: richard.ll...@connectinternetsolutions.com Connect Internet Solutions,WWW: https://www.connectinternetsolutions.com/ 4th Floor, New Barratt House, 47, North John Street, Liverpool, Merseyside, UK. L2 6SG -Peff -- This e-mail (and any attachments) is private and confidential. If you have received it in error, please notify the sender immediately and delete it from your system. Do not use, copy or disclose the information in any way nor act in reliance on it. Any views expressed in this message are those of the individual sender, except where the sender specifically states them to be the views of Connect Internet Solutions Ltd. This e-mail and any attachments are believed to be virus free but it is the recipient's responsibility to ensure that they are. Connect Internet Solutions Ltd (A company registered in England No: 04424350) Registered Office: 4th Floor, New Barratt House, 47 North John Street, Liverpool, L2 6SG Telephone: +44 (0) 151 282 4321 VAT registration number: 758 2838 85
[PATCH v2 3/3] batch check whether submodule needs pushing into one call
We run a command for each sha1 change in a submodule. This is unnecessary since we can simply batch all sha1's we want to check into one command. Lets do it so we can speedup the check when many submodule changes are in need of checking. Signed-off-by: Heiko Voigt --- submodule.c | 63 + 1 file changed, 34 insertions(+), 29 deletions(-) diff --git a/submodule.c b/submodule.c index 5044afc2f8..a05c2a34b1 100644 --- a/submodule.c +++ b/submodule.c @@ -529,27 +529,49 @@ static int append_hash_to_argv(const unsigned char sha1[20], void *data) return 0; } -static int submodule_needs_pushing(const char *path, const unsigned char sha1[20]) +static int check_has_hash(const unsigned char sha1[20], void *data) { - if (add_submodule_odb(path) || !lookup_commit_reference(sha1)) + int *has_hash = (int *) data; + + if (!lookup_commit_reference(sha1)) + *has_hash = 0; + + return 0; +} + +static int submodule_has_hashes(const char *path, struct sha1_array *hashes) +{ + int has_hash = 1; + + if (add_submodule_odb(path)) + return 0; + + sha1_array_for_each_unique(hashes, check_has_hash, &has_hash); + return has_hash; +} + +static int submodule_needs_pushing(const char *path, struct sha1_array *hashes) +{ + if (!submodule_has_hashes(path, hashes)) return 0; if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) { struct child_process cp = CHILD_PROCESS_INIT; - const char *argv[] = {"rev-list", NULL, "--not", "--remotes", "-n", "1" , NULL}; struct strbuf buf = STRBUF_INIT; int needs_pushing = 0; - argv[1] = sha1_to_hex(sha1); - cp.argv = argv; + argv_array_push(&cp.args, "rev-list"); + sha1_array_for_each_unique(hashes, append_hash_to_argv, &cp.args); + argv_array_pushl(&cp.args, "--not", "--remotes", "-n", "1" , NULL); + prepare_submodule_repo_env(&cp.env_array); cp.git_cmd = 1; cp.no_stdin = 1; cp.out = -1; cp.dir = path; if (start_command(&cp)) - die("Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s", - sha1_to_hex(sha1), path); + die("Could not run 'git rev-list --not --remotes -n 1' command in submodule %s", + path); if (strbuf_read(&buf, cp.out, 41)) needs_pushing = 1; finish_command(&cp); @@ -604,21 +626,6 @@ static void find_unpushed_submodule_commits(struct commit *commit, diff_tree_combined_merge(commit, 1, &rev); } -struct collect_submodule_from_sha1s_data { - char *submodule_path; - struct string_list *needs_pushing; -}; - -static void collect_submodules_from_sha1s(const unsigned char sha1[20], - void *data) -{ - struct collect_submodule_from_sha1s_data *me = - (struct collect_submodule_from_sha1s_data *) data; - - if (submodule_needs_pushing(me->submodule_path, sha1)) - string_list_insert(me->needs_pushing, me->submodule_path); -} - static void free_submodules_sha1s(struct string_list *submodules) { int i; @@ -658,13 +665,11 @@ int find_unpushed_submodules(struct sha1_array *hashes, argv_array_clear(&argv); for (i = 0; i < submodules.nr; i++) { - struct string_list_item *item = &submodules.items[i]; - struct collect_submodule_from_sha1s_data data; - data.submodule_path = item->string; - data.needs_pushing = needs_pushing; - sha1_array_for_each_unique((struct sha1_array *) item->util, - collect_submodules_from_sha1s, - &data); + struct string_list_item *submodule = &submodules.items[i]; + struct sha1_array *hashes = (struct sha1_array *) submodule->util; + + if (submodule_needs_pushing(submodule->string, hashes)) + string_list_insert(needs_pushing, submodule->string); } free_submodules_sha1s(&submodules); -- 2.10.1.637.g09b28c5
[PATCH v2 2/3] serialize collection of refs that contain submodule changes
We are iterating over each pushed ref and want to check whether it contains changes to submodules. Instead of immediately checking each ref lets first collect them and then do the check for all of them in one revision walk. Signed-off-by: Heiko Voigt --- submodule.c | 36 +--- submodule.h | 5 +++-- transport.c | 29 + 3 files changed, 45 insertions(+), 25 deletions(-) diff --git a/submodule.c b/submodule.c index 59c9d15905..5044afc2f8 100644 --- a/submodule.c +++ b/submodule.c @@ -522,6 +522,13 @@ static int has_remote(const char *refname, const struct object_id *oid, return 1; } +static int append_hash_to_argv(const unsigned char sha1[20], void *data) +{ + struct argv_array *argv = (struct argv_array *) data; + argv_array_push(argv, sha1_to_hex(sha1)); + return 0; +} + static int submodule_needs_pushing(const char *path, const unsigned char sha1[20]) { if (add_submodule_odb(path) || !lookup_commit_reference(sha1)) @@ -623,24 +630,24 @@ static void free_submodules_sha1s(struct string_list *submodules) string_list_clear(submodules, 1); } -int find_unpushed_submodules(unsigned char new_sha1[20], +int find_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name, struct string_list *needs_pushing) { struct rev_info rev; struct commit *commit; - const char *argv[] = {NULL, NULL, "--not", "NULL", NULL}; - int argc = ARRAY_SIZE(argv) - 1, i; - char *sha1_copy; + int i; struct string_list submodules = STRING_LIST_INIT_DUP; + struct argv_array argv = ARGV_ARRAY_INIT; - struct strbuf remotes_arg = STRBUF_INIT; - - strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name); init_revisions(&rev, NULL); - sha1_copy = xstrdup(sha1_to_hex(new_sha1)); - argv[1] = sha1_copy; - argv[3] = remotes_arg.buf; - setup_revisions(argc, argv, &rev, NULL); + + /* argv.argv[0] will be ignored by setup_revisions */ + argv_array_push(&argv, "find_unpushed_submodules"); + sha1_array_for_each_unique(hashes, append_hash_to_argv, &argv); + argv_array_push(&argv, "--not"); + argv_array_pushf(&argv, "--remotes=%s", remotes_name); + + setup_revisions(argv.argc, argv.argv, &rev, NULL); if (prepare_revision_walk(&rev)) die("revision walk setup failed"); @@ -648,8 +655,7 @@ int find_unpushed_submodules(unsigned char new_sha1[20], find_unpushed_submodule_commits(commit, &submodules); reset_revision_walk(); - free(sha1_copy); - strbuf_release(&remotes_arg); + argv_array_clear(&argv); for (i = 0; i < submodules.nr; i++) { struct string_list_item *item = &submodules.items[i]; @@ -687,12 +693,12 @@ static int push_submodule(const char *path) return 1; } -int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name) +int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name) { int i, ret = 1; struct string_list needs_pushing = STRING_LIST_INIT_DUP; - if (!find_unpushed_submodules(new_sha1, remotes_name, &needs_pushing)) + if (!find_unpushed_submodules(hashes, remotes_name, &needs_pushing)) return 1; for (i = 0; i < needs_pushing.nr; i++) { diff --git a/submodule.h b/submodule.h index d9e197a948..065b2f0a2a 100644 --- a/submodule.h +++ b/submodule.h @@ -3,6 +3,7 @@ struct diff_options; struct argv_array; +struct sha1_array; enum { RECURSE_SUBMODULES_CHECK = -4, @@ -62,9 +63,9 @@ int submodule_uses_gitfile(const char *path); int ok_to_remove_submodule(const char *path); int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20], const unsigned char a[20], const unsigned char b[20], int search); -int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name, +int find_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name, struct string_list *needs_pushing); -int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name); +int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name); void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir); int parallel_submodules(void); diff --git a/transport.c b/transport.c index 94d6dc3725..05f2ce83f1 100644 --- a/transport.c +++ b/transport.c @@ -903,23 +903,36 @@ int transport_push(struct transport *transport, if ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) { struct ref *ref = remote_refs; + struct sha1_array hashes = SHA1_ARRAY_INIT; + for (; ref; ref = ref->next) - if (!is_null_oid(&ref->n
[PATCH v2 1/3] serialize collection of changed submodules
To check whether a submodule needs to be pushed we need to collect all changed submodules. Lets collect them first and then execute the possibly expensive test whether certain revisions are already pushed only once per submodule. There is further potential for optimization since we can assemble one command and only issued that instead of one call for each remote ref in the submodule. Signed-off-by: Heiko Voigt --- submodule.c | 63 - 1 file changed, 58 insertions(+), 5 deletions(-) diff --git a/submodule.c b/submodule.c index 2de06a3351..59c9d15905 100644 --- a/submodule.c +++ b/submodule.c @@ -554,19 +554,34 @@ static int submodule_needs_pushing(const char *path, const unsigned char sha1[20 return 0; } +static struct sha1_array *get_sha1s_from_list(struct string_list *submodules, + const char *path) +{ + struct string_list_item *item; + + item = string_list_insert(submodules, path); + if (item->util) + return (struct sha1_array *) item->util; + + /* NEEDSWORK: should we have sha1_array_init()? */ + item->util = xcalloc(1, sizeof(struct sha1_array)); + return (struct sha1_array *) item->util; +} + static void collect_submodules_from_diff(struct diff_queue_struct *q, struct diff_options *options, void *data) { int i; - struct string_list *needs_pushing = data; + struct string_list *submodules = data; for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; + struct sha1_array *hashes; if (!S_ISGITLINK(p->two->mode)) continue; - if (submodule_needs_pushing(p->two->path, p->two->oid.hash)) - string_list_insert(needs_pushing, p->two->path); + hashes = get_sha1s_from_list(submodules, p->two->path); + sha1_array_append(hashes, p->two->oid.hash); } } @@ -582,14 +597,41 @@ static void find_unpushed_submodule_commits(struct commit *commit, diff_tree_combined_merge(commit, 1, &rev); } +struct collect_submodule_from_sha1s_data { + char *submodule_path; + struct string_list *needs_pushing; +}; + +static void collect_submodules_from_sha1s(const unsigned char sha1[20], + void *data) +{ + struct collect_submodule_from_sha1s_data *me = + (struct collect_submodule_from_sha1s_data *) data; + + if (submodule_needs_pushing(me->submodule_path, sha1)) + string_list_insert(me->needs_pushing, me->submodule_path); +} + +static void free_submodules_sha1s(struct string_list *submodules) +{ + int i; + for (i = 0; i < submodules->nr; i++) { + struct string_list_item *item = &submodules->items[i]; + struct sha1_array *hashes = (struct sha1_array *) item->util; + sha1_array_clear(hashes); + } + string_list_clear(submodules, 1); +} + int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name, struct string_list *needs_pushing) { struct rev_info rev; struct commit *commit; const char *argv[] = {NULL, NULL, "--not", "NULL", NULL}; - int argc = ARRAY_SIZE(argv) - 1; + int argc = ARRAY_SIZE(argv) - 1, i; char *sha1_copy; + struct string_list submodules = STRING_LIST_INIT_DUP; struct strbuf remotes_arg = STRBUF_INIT; @@ -603,12 +645,23 @@ int find_unpushed_submodules(unsigned char new_sha1[20], die("revision walk setup failed"); while ((commit = get_revision(&rev)) != NULL) - find_unpushed_submodule_commits(commit, needs_pushing); + find_unpushed_submodule_commits(commit, &submodules); reset_revision_walk(); free(sha1_copy); strbuf_release(&remotes_arg); + for (i = 0; i < submodules.nr; i++) { + struct string_list_item *item = &submodules.items[i]; + struct collect_submodule_from_sha1s_data data; + data.submodule_path = item->string; + data.needs_pushing = needs_pushing; + sha1_array_for_each_unique((struct sha1_array *) item->util, + collect_submodules_from_sha1s, + &data); + } + free_submodules_sha1s(&submodules); + return needs_pushing->nr; } -- 2.10.1.637.g09b28c5
[PATCH v2 0/3] Speedup finding of unpushed submodules
You can find the first iteration of this series as part of this thread: http://public-inbox.org/git/%3C20160914173124.GA7613@sandbox%3E/ All mentioned issues should be fixed. I dropped the last patch which was the cause of the broken tests. This should optimize every part of this test to a nice speed if you are pushing to a remote. The only case that is still broken/slow as hell is when calling push with a direct url. I am thinking whether we should maybe error out with a "not implemented" message or something and mention that --recurse-submoules does not work with direct urls? But we might want to have another look at performance with this patch included. Maybe it is actually useable with the last patch included which was not yet on pu. Cheers Heiko Heiko Voigt (3): serialize collection of changed submodules serialize collection of refs that contain submodule changes batch check whether submodule needs pushing into one call submodule.c | 116 ++-- submodule.h | 5 +-- transport.c | 29 ++- 3 files changed, 114 insertions(+), 36 deletions(-) -- 2.10.1.637.g09b28c5
Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
Johannes Schindelin writes: > Hi Matthieu, > > On Fri, 7 Oct 2016, Matthieu Moy wrote: > >> Another possibility: !(nocd), which leaves room >> for !(keyword1,keyword2,...) if needed later. Also, it is consistent >> with the :(word) syntax of pathspecs. > > But is this backwards-compatible? Don't we execute everything that comes > after the exclamation mark as a command-line via shell, where the > parentheses mean "open a subshell"? Good point. I can imagine someone already having [alias] foo = !(cd bar && $something) && $something_else Your proposed (keyword)! is better. -- Matthieu Moy http://www-verimag.imag.fr/~moy/
Re: [PATCH] clean up confusing suggestion for commit references
On Fri, Oct 07, 2016 at 11:56:38AM +0200, Heiko Voigt wrote: > The description for referencing commits looks as if it is contradicting > the example, since it is itself enclosed in double quotes. Lets use > single quotes around the description and include the double quotes in > the description so it matches the example. > --- > Sorry for opening this up again but I just looked up the format and was > like: "Umm, which one is now the correct one..." > > For this makes more sense. What do others think? Looking over the threads, I wasn't sure there was consensus[1,2]. So it would be equally correct to drop the quotes from the example. I dunno. I am in favor of no-quotes, myself, so maybe I am just manufacturing dissent in my mind. :) -Peff [1] http://public-inbox.org/git/a9731f60-5c30-0bc6-f73a-f7ffb7bd4...@kdbg.org/ [2] http://public-inbox.org/git/20160829183015.2uqnfezekjfa3...@sigill.intra.peff.net/
Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
Hi Kuba, On Fri, 7 Oct 2016, Jakub Narębski wrote: > W dniu 07.10.2016 o 13:20, Johannes Schindelin pisze: > > On Thu, 6 Oct 2016, Junio C Hamano wrote: > >> Nguyễn Thái Ngọc Duy writes: > >> > >>> Throwing something at the mailing list to see if anybody is > >>> interested. > >>> > >>> Current '!' aliases move cwd to $GIT_WORK_TREE first, which could make > >>> handling path arguments hard because they are relative to the original > >>> cwd. We set GIT_PREFIX to work around it, but I still think it's more > >>> natural to keep cwd where it is. > >>> > >>> We have a way to do that now after 441981b (git: simplify environment > >>> save/restore logic - 2016-01-26). It's just a matter of choosing the > >>> right syntax. I'm going with '!!'. I'm not very happy with it. But I > >>> do like this type of alias. > >> > >> I do not know why you are not happy with the syntax, but I > >> personally think it brilliant, both the idea and the preliminary > >> clean-up that made this possible with a simple patch like this. > > > > I guess he is not happy with it because "!!" is quite unintuitive a > > construct. I know that *I* would have been puzzled by it, asking "What the > > heck does this do?". > > Well, "!" as a prefix is not intuitive either. You do not use vi, do you? :-P In vi, if you enter command mode (typing a colon) and then want to execute, say, `pwd`, you type !pwd > Perhaps "!.", because "." is current directory, and the "." command > (that is, alias to "source") doesn't make sense in git aliases. If you want to execute, say, `pwd` in the current directory, that would mean you want to write !.pwd But that already means "execute `.pwd`"... > Note that we would have to teach git completion about new syntax; > or new configuration variable if we go that route. Why would we? Git's completion does not expand aliases, it only completes the aliases' names, not their values. Ciao, Dscho
Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
On Fri, Oct 07, 2016 at 04:11:34PM +0200, Johannes Schindelin wrote: > Possibly a better idea would be to use *another* special symbol, one that > makes intuitive sense as a modifier, such as: > > [alias] > # This works as before > xyz = !pwd > # As does this > stat = -p status > # This, however, is different: > duy = (nocd)!pwd > > This is backwards compatible as "(" is not a part of any Git command, nor > of a valid alias, nor is it commonly used as part of a git-* > executable/script. > > It is also kind of a bit more intuitive, I'd wager, and it is also > extensible to future options we may want to introduce. I like this much better (like you, I am concerned about things like "!(foo)" as conflicting with the shell). And I think your "(nocd)!pwd" example is quite readable. -Peff
Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
Hi Matthieu, On Fri, 7 Oct 2016, Matthieu Moy wrote: > Duy Nguyen writes: > > > On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin > > wrote: > >> Hi Junio, > >> > >> On Thu, 6 Oct 2016, Junio C Hamano wrote: > >> > >>> Nguyễn Thái Ngọc Duy writes: > >>> > >>> > Throwing something at the mailing list to see if anybody is > >>> > interested. > >>> > > >>> > Current '!' aliases move cwd to $GIT_WORK_TREE first, which could > >>> > make > >>> > handling path arguments hard because they are relative to the > >>> > original > >>> > cwd. We set GIT_PREFIX to work around it, but I still think it's > >>> > more > >>> > natural to keep cwd where it is. > >>> > > >>> > We have a way to do that now after 441981b (git: simplify > >>> > environment > >>> > save/restore logic - 2016-01-26). It's just a matter of choosing > >>> > the > >>> > right syntax. I'm going with '!!'. I'm not very happy with it. > >>> > But I > >>> > do like this type of alias. > >>> > >>> I do not know why you are not happy with the syntax, but I > >>> personally think it brilliant, both the idea and the preliminary > >>> clean-up that made this possible with a simple patch like this. > >> > >> I guess he is not happy with it because "!!" is quite unintuitive a > >> construct. I know that *I* would have been puzzled by it, asking > >> "What the > >> heck does this do?". > > > > Yep. And I wouldn't want to set a tradition for the next alias type > > '!!!'. There's no good choice to represent a new alias type with a > > leading symbol. This just occurred to me, however, what do you think > > about a new config group for it? With can have something like > > externalAlias.* (or some other name) that lives in parallel with > > alias.*. Then we don't need '!' (or '!!') at all. > > Another possibility: !(nocd), which leaves room > for !(keyword1,keyword2,...) if needed later. Also, it is consistent > with the :(word) syntax of pathspecs. But is this backwards-compatible? Don't we execute everything that comes after the exclamation mark as a command-line via shell, where the parentheses mean "open a subshell"? Ciao, Dscho
Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
Hi Duy, On Fri, 7 Oct 2016, Duy Nguyen wrote: > On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin > wrote: > > > > On Thu, 6 Oct 2016, Junio C Hamano wrote: > > > >> Nguyễn Thái Ngọc Duy writes: > >> > >> > Throwing something at the mailing list to see if anybody is > >> > interested. > >> > > >> > Current '!' aliases move cwd to $GIT_WORK_TREE first, which could make > >> > handling path arguments hard because they are relative to the original > >> > cwd. We set GIT_PREFIX to work around it, but I still think it's more > >> > natural to keep cwd where it is. > >> > > >> > We have a way to do that now after 441981b (git: simplify environment > >> > save/restore logic - 2016-01-26). It's just a matter of choosing the > >> > right syntax. I'm going with '!!'. I'm not very happy with it. But I > >> > do like this type of alias. > >> > >> I do not know why you are not happy with the syntax, but I > >> personally think it brilliant, both the idea and the preliminary > >> clean-up that made this possible with a simple patch like this. > > > > I guess he is not happy with it because "!!" is quite unintuitive a > > construct. I know that *I* would have been puzzled by it, asking "What the > > heck does this do?". > > Yep. And I wouldn't want to set a tradition for the next alias type > '!!!'. There's no good choice to represent a new alias type with a > leading symbol. This just occurred to me, however, what do you think > about a new config group for it? With can have something like > externalAlias.* (or some other name) that lives in parallel with > alias.*. Then we don't need '!' (or '!!') at all. But what would the precedence be? externalAlias.xyz wins over alias.xyz? And we still would need '!' support: tons of people (including myself) rely on it. Possibly a better idea would be to use *another* special symbol, one that makes intuitive sense as a modifier, such as: [alias] # This works as before xyz = !pwd # As does this stat = -p status # This, however, is different: duy = (nocd)!pwd This is backwards compatible as "(" is not a part of any Git command, nor of a valid alias, nor is it commonly used as part of a git-* executable/script. It is also kind of a bit more intuitive, I'd wager, and it is also extensible to future options we may want to introduce. Ciao, Dscho
Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
Hello, Johannes W dniu 07.10.2016 o 13:20, Johannes Schindelin pisze: > On Thu, 6 Oct 2016, Junio C Hamano wrote: >> Nguyễn Thái Ngọc Duy writes: >> >>> Throwing something at the mailing list to see if anybody is >>> interested. >>> >>> Current '!' aliases move cwd to $GIT_WORK_TREE first, which could make >>> handling path arguments hard because they are relative to the original >>> cwd. We set GIT_PREFIX to work around it, but I still think it's more >>> natural to keep cwd where it is. >>> >>> We have a way to do that now after 441981b (git: simplify environment >>> save/restore logic - 2016-01-26). It's just a matter of choosing the >>> right syntax. I'm going with '!!'. I'm not very happy with it. But I >>> do like this type of alias. >> >> I do not know why you are not happy with the syntax, but I >> personally think it brilliant, both the idea and the preliminary >> clean-up that made this possible with a simple patch like this. > > I guess he is not happy with it because "!!" is quite unintuitive a > construct. I know that *I* would have been puzzled by it, asking "What the > heck does this do?". Well, "!" as a prefix is not intuitive either. Perhaps "!.", because "." is current directory, and the "." command (that is, alias to "source") doesn't make sense in git aliases. Note that we would have to teach git completion about new syntax; or new configuration variable if we go that route. -- Jakub Narębski
Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
Duy Nguyen writes: > On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin > wrote: >> Hi Junio, >> >> On Thu, 6 Oct 2016, Junio C Hamano wrote: >> >>> Nguyễn Thái Ngọc Duy writes: >>> >>> > Throwing something at the mailing list to see if anybody is >>> > interested. >>> > >>> > Current '!' aliases move cwd to $GIT_WORK_TREE first, which could >>> > make >>> > handling path arguments hard because they are relative to the >>> > original >>> > cwd. We set GIT_PREFIX to work around it, but I still think it's >>> > more >>> > natural to keep cwd where it is. >>> > >>> > We have a way to do that now after 441981b (git: simplify >>> > environment >>> > save/restore logic - 2016-01-26). It's just a matter of choosing >>> > the >>> > right syntax. I'm going with '!!'. I'm not very happy with it. >>> > But I >>> > do like this type of alias. >>> >>> I do not know why you are not happy with the syntax, but I >>> personally think it brilliant, both the idea and the preliminary >>> clean-up that made this possible with a simple patch like this. >> >> I guess he is not happy with it because "!!" is quite unintuitive a >> construct. I know that *I* would have been puzzled by it, asking >> "What the >> heck does this do?". > > Yep. And I wouldn't want to set a tradition for the next alias type > '!!!'. There's no good choice to represent a new alias type with a > leading symbol. This just occurred to me, however, what do you think > about a new config group for it? With can have something like > externalAlias.* (or some other name) that lives in parallel with > alias.*. Then we don't need '!' (or '!!') at all. Another possibility: !(nocd), which leaves room for !(keyword1,keyword2,...) if needed later. Also, it is consistent with the :(word) syntax of pathspecs. -- Matthieu Moy http://www-verimag.imag.fr/~moy/
Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION
Junio C Hamano writes: > Sergey Organov writes: > Last, if "reference" is not good enough and we get to internals anyway, why not say SHA1 then? >>> >>> Because that is still colloquial? I think s/name/object name/ is a >>> sensible change, but not s/name/reference/. >> >> No, "reference" is more sensible here than any of "name", "object name", >> or "SHA-1", the same way as here: >> >> $ git help glossary >> [...] >> chain >> A list of objects, where each object in the list contains a >> reference to its successor (for example, the successor of a >> commit could be one of its parents). >> [...] > > The entry for "chain" and the description under discussion have > stress on different aspect, though. The description of "chain" is > more general: an object refers to another object by referring to it, > by unspecified means. The reason why it is left unspecified is > because the way a tree object refers to blobs and trees is different > from the way a commit object refers to its parents (the former has > object names of blobs and trees in the tree entries; the latter uses > "parent" entries in the object header part to record object names of > parent commits). It wants to stress more on the fact that there is > some mechanism to associate one object to others, than how that > association/linkage is expressed. > > The way the resulting commit is described in the original text of > "git merge" description stresses more on "how" by being a lot more > specific to commit objects. It does not just say "refers to parents > (by unspecified means)"; instead it tries to say what exactly are > recorded, i.e. the parents are referred to by recording the object > names of them in a new commit object. It stresses more on "how" > (because it can afford to be more specific, unlike the description > of more general concept of a "chain"). That's were our disagreement actually is, and that's what I've tried to fix with s/name/reference/, and that's why I'm against s/name/object name/. Rather than being more (and more) specific at every opportunity, one needs a good reason to get more specific. In this particular case, general DAG terminology seems to be enough to describe git-merge semantics, thus using GIT specifics is unfounded. > It may be debatable if we want to give the description of what is > exactly recorded at that point of the document, Exactly. My point in this particular discussion is that details of recording of references to parents don't belong here, even though to tell the truth I think they don't belong to git _user_ documentation at all. > but I personally > think that the users deserve a chance to learn how a merge is > recorded in "git merge" documentation. I doubt a user will gain anything from this sacred knowledge suddenly being thrown on him when what she is looking for is understanding of basic merge semantics in GIT. That said, if you still disagree, please feel free to just drop the patch. -- Sergey
Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
On Fri, Oct 7, 2016 at 2:15 AM, Junio C Hamano wrote: > Duy Nguyen writes: > >> On Tue, Oct 4, 2016 at 11:15 PM, Junio C Hamano wrote: >>> Duy Nguyen writes: >>> We don't use it internally _yet_. I need to go through all the external diff code and see --shift-ita should be there. The end goal is still changing the default behavior and getting rid of --shift-ita, >>> >>> I do not agree with that endgame, and quite honestly I do not want >>> to waste time reviewing such a series. > > I definitely shouldn't have said that, especially "waste". Many > issues around i-t-a and diff make my head hurt when I think about > them [*1*], but not wanting to spend time that gets my > head hurt and not wanting to waste time are totally different > things. My apologies. No problem. I do appreciate a straight shoot down though. Many of my topics have been going on for months (ones not in 'pu') and seeing it rejected near the end is worse than stopping working on them early. > I missed something curious in your statement above, i.e. "external > diff". I thought we have pretty much got rid of all the invocation > of "git diff" via the run_command() interface and we do not need the > command line option (we only need the options->shift_ita so that > callers like "git status" can seletively ask for it when making > internal calls), and that is why I didn't want to see it. I don't know if we have had any external diff calls in our shell-based commands. I don't read them often. Regardless, people do use "git diff" and it should show the "right thing" (I know it's subjective). Or at least be consistent with both git-commit and git-status. > [Footnote] > > Here is one of the things around i-t-a and diff. If you make "git > diff" (between the index and the working tree) report "new" file, it > would imply that "git apply" run without "--index" should create an Off topic. This reminds me of an old patch about apply and ita [1] but that one is not the same here > ita entry in the index for symmetry, wouldn't it? That by itself > can be seen as an improvement (we no longer would have to say that > "git apply patchfile && git commit -a" that is run in a clean state > will forget new files the patchfile creates), but it also means we > now need a repository in order to run "git apply" (without "--index"), > which is a problem, as "git apply" is often used as a better "patch". We could detect "no repo available" and ignore the index, I guess. > "git apply --cached" may also become "interesting". A patch that > would apply cleanly to HEAD should apply cleanly if you did this: > > $ git read-tree HEAD > $ git apply --cached > no matter what the working tree state is. Should a patch that > creates a "new" file add contents to the index, or just an i-t-a > entry? I could argue it both ways, but either is quite satisfactory > and makes my head hurt. --cached tells you to put new contents in the index. I-ta entries, being a reminder to add stuff, don't really fit in here because you want to add contents _now_, i think. After a successful "git apply --cached", a "git commit" should contain exactly what the applied patch has. If new files are i-t-a entries instead, then the new commit would not be the same as the patch. [1] https://public-inbox.org/git/1451181092-26054-4-git-send-email-pclo...@gmail.com/ -- Duy
Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
On Fri, Oct 7, 2016 at 7:47 PM, Matthieu Moy wrote: > Duy Nguyen writes: > >> On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin >> wrote: >>> Hi Junio, >>> >>> On Thu, 6 Oct 2016, Junio C Hamano wrote: >>> Nguyễn Thái Ngọc Duy writes: > Throwing something at the mailing list to see if anybody is > interested. > > Current '!' aliases move cwd to $GIT_WORK_TREE first, which could > make > handling path arguments hard because they are relative to the > original > cwd. We set GIT_PREFIX to work around it, but I still think it's > more > natural to keep cwd where it is. > > We have a way to do that now after 441981b (git: simplify > environment > save/restore logic - 2016-01-26). It's just a matter of choosing > the > right syntax. I'm going with '!!'. I'm not very happy with it. > But I > do like this type of alias. I do not know why you are not happy with the syntax, but I personally think it brilliant, both the idea and the preliminary clean-up that made this possible with a simple patch like this. >>> >>> I guess he is not happy with it because "!!" is quite unintuitive a >>> construct. I know that *I* would have been puzzled by it, asking >>> "What the >>> heck does this do?". >> >> Yep. And I wouldn't want to set a tradition for the next alias type >> '!!!'. There's no good choice to represent a new alias type with a >> leading symbol. This just occurred to me, however, what do you think >> about a new config group for it? With can have something like >> externalAlias.* (or some other name) that lives in parallel with >> alias.*. Then we don't need '!' (or '!!') at all. > > Another possibility: !(nocd), which leaves room > for !(keyword1,keyword2,...) if needed later. Also, it is consistent > with the :(word) syntax of pathspecs. This seems to solve my problem nicely. -- Duy
Re: [PATCH 1/2] submodule add: extend force flag to add existing repos
On Thu, Oct 06, 2016 at 01:11:20PM -0700, Junio C Hamano wrote: > Stefan Beller writes: > > > Currently the force flag in `git submodule add` takes care of possibly > > ignored files or when a name collision occurs. > > > > However there is another situation where submodule add comes in handy: > > When you already have a gitlink recorded, but no configuration was > > done (i.e. no .gitmodules file nor any entry in .git/config) and you > > want to generate these config entries. For this situation allow > > `git submodule add` to proceed if there is already a submodule at the > > given path in the index. Is it important that the submodule is in the index? How about worktree? >From the index entry alone we can not deduce the values anyway. So I would say the submodule has to be in the worktree, no matter what is in the index. If its not in the index we can also add it. BTW, that is the way I imagined submodules would work in the first place: just clone and add them, like I described here[1]. > > Signed-off-by: Stefan Beller > > --- > > Yup, the goal makes perfect sense. > > I vaguely recall discussing this exact issue of "git submodule add" > that refuses to add a path that already is a gitlink (via "git add" > that has previously been run) elsewhere on this list some time ago. Yes there was a discussion, see the link. Cheers Heiko [1] http://public-inbox.org/git/%3c20160916141143.ga47...@book.hvoigt.net%3E/
Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist
On Thu, Oct 06, 2016 at 10:20:16AM -0700, Stefan Beller wrote: > On Thu, Oct 6, 2016 at 2:23 AM, Heiko Voigt wrote: > > On Wed, Oct 05, 2016 at 03:53:25PM +0200, Heiko Voigt wrote: > >> On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote: > >> > Jeff, > >> > thanks for the suggestions, both git_path(..) as well as checking the > >> > config, > >> > this seems quite readable to me: > >> > >> When reading the discussion I thought the same: What about the > >> "old-style" repositories. I like this one. Checking both locations > >> is nice. > > > > BTW, since it seems we all agree on the direction. Should we add some > > tests? > > > > Good call. What do we want to test for? > * Correctness in case of submodules? (just push and get rejected) > I think that is easy to do. > * Performance with [no, lots of] submodules? That seems harder to me. > > I'll add a test for the correctness part and resend. Well I though about the following tests: * Without submodules: Make sure submodule processing is disabled by default * With new-style submodules: Make sure submodules are processed by default * With old-style submodules: Make sure submodules are processed by default But I have to admit that I did not think about the "how can we do that". But performance seems to be the only thing that is exposing the processing when we have no submodules, so it seems we can only easily do the tests with submodules. Cheers Heiko
Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin wrote: > Hi Junio, > > On Thu, 6 Oct 2016, Junio C Hamano wrote: > >> Nguyễn Thái Ngọc Duy writes: >> >> > Throwing something at the mailing list to see if anybody is >> > interested. >> > >> > Current '!' aliases move cwd to $GIT_WORK_TREE first, which could make >> > handling path arguments hard because they are relative to the original >> > cwd. We set GIT_PREFIX to work around it, but I still think it's more >> > natural to keep cwd where it is. >> > >> > We have a way to do that now after 441981b (git: simplify environment >> > save/restore logic - 2016-01-26). It's just a matter of choosing the >> > right syntax. I'm going with '!!'. I'm not very happy with it. But I >> > do like this type of alias. >> >> I do not know why you are not happy with the syntax, but I >> personally think it brilliant, both the idea and the preliminary >> clean-up that made this possible with a simple patch like this. > > I guess he is not happy with it because "!!" is quite unintuitive a > construct. I know that *I* would have been puzzled by it, asking "What the > heck does this do?". Yep. And I wouldn't want to set a tradition for the next alias type '!!!'. There's no good choice to represent a new alias type with a leading symbol. This just occurred to me, however, what do you think about a new config group for it? With can have something like externalAlias.* (or some other name) that lives in parallel with alias.*. Then we don't need '!' (or '!!') at all. -- Duy
Re: [PATCH 6/6] Documentation/git-merge.txt: get rid of irrelevant references to git-pull
Junio C Hamano writes: > Sergey Organov writes: > >> Ah, I now see. I tried to keep the text intact as much as possible, and >> only split it into description and a note. Well, how about this then: > > Much better than your earlier patch, but I am not sure if the > updated one is that much better compared to the original. It's not intended to be much better. It is aimed at single simple target: get rid of git-pull from descriptions of operations of git-merge. I'd just remove those git-pull reference, the only one that is left after the patch, but it looks like git-merge needs an excuse to have fast-forward on by default, and that excuse is the common git-pull case. [I'd prefer 'git-merge --ff' were called from 'git-pull' and --no-ff be the default for git-merge, but that's not the case, so I left the reference to git-pull intact.] > > The pre- and post- state of this "how about this" patch essentially > say the same thing, and I suspect that the primary reason why you > think the post- state is easier to read is because you wrote it, > while the reason why I do not see much difference is because I > didn't write the updated one ;-). > > I do find "In this case, ... store the combined history" in the > original a bit awkward to read, but most of that awkardness is > inherited by the updated text. It may benefit from hinting why a > new commit is not needed a bit stronger. Here is my attempt: > > When the commit we are merging is a descendant of the current > HEAD, the history leading to the named commit can be, and by > default is, taken as the combined history of the two. Our > history is "fast forwarded" to their history by updating `HEAD` > along with the index to point at the named commit. > > This often happens when you are following along somebody else's > work via "git pull" without doing your own development. > > I think the awkwardness I felt in the original and your version is > gone from the above attempt, but I doubt that it is better over > either of them in any other way. This is entirely different matter, and should be a subject of another patch, if any. My patch meant to only address git-pull references, with as few changes as possible. -- Sergey
Re: Regression: git no longer works with musl libc's regex impl
W dniu 07.10.2016 o 00:42, Ramsay Jones pisze: > On 06/10/16 20:18, Ævar Arnfjörð Bjarmason wrote: [...] >> But just to clarify, does anyone have any objection to making our >> configure.ac compile a C program to check for this sort of thing? >> Because that seems like the easiest solution to this class of problem. > > Err, you do know that we already do that, right? > > [see commit a1e3b669 ("autoconf: don't use platform regex if it lacks > REG_STARTEND", 17-08-2010)] > > In fact, if you run the auto tools on cygwin, you get a different setting > for NO_REGEX than via config.mak.uname. Which is why I don't run configure > on cygwin. :-D > > [The issue is exposed by t7008-grep-binary.sh, where the cygwin native > regex library matches '.' in a pattern with the NUL character. ie the > test_expect_failure test passes.] Huh. So we have NO_REGEX support in ./configure, and people using Git on untypical architectures and systems *can* make use of it. It was just described wrongly, so in turn to have the more neutral description, the same as in Makefile, let's do this: >8 -- >8 - >8 -- >8 -- Subject: [PATCH] configure.ac: Improve description of NO_REGEX test The commit 2f8952250a changed description of NO_REGEX build config variable to be more neutral, and actually say that it is about support for REG_STARTEND. Change description in configure.ac to be the same. Change also the test message and variable name to match. The test just checks that REG_STARTEND is #defined. Issue-found-by: Ramsay Jones Signed-off-by: Jakub Narębski --- configure.ac | 13 +++-- 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/configure.ac b/configure.ac index aa9c91d..7f39fd0 100644 --- a/configure.ac +++ b/configure.ac @@ -835,9 +835,10 @@ AC_CHECK_TYPE([struct addrinfo],[ ]) GIT_CONF_SUBST([NO_IPV6]) # -# Define NO_REGEX if you have no or inferior regex support in your C library. -AC_CACHE_CHECK([whether the platform regex can handle null bytes], - [ac_cv_c_excellent_regex], [ +# Define NO_REGEX if your C library lacks regex support with REG_STARTEND +# feature. +AC_CACHE_CHECK([whether the platform regex supports REG_STARTEND], + [ac_cv_c_regex_with_reg_startend], [ AC_EGREP_CPP(yippeeyeswehaveit, AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT #include @@ -846,10 +847,10 @@ AC_EGREP_CPP(yippeeyeswehaveit, yippeeyeswehaveit #endif ]), - [ac_cv_c_excellent_regex=yes], - [ac_cv_c_excellent_regex=no]) + [ac_cv_c_regex_with_reg_startend=yes], + [ac_cv_c_regex_with_reg_startend=no]) ]) -if test $ac_cv_c_excellent_regex = yes; then +if test $ac_cv_c_regex_with_reg_startend = yes; then NO_REGEX= else NO_REGEX=YesPlease -- 2.10.0
Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
Hi Junio, On Thu, 6 Oct 2016, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > > > Throwing something at the mailing list to see if anybody is > > interested. > > > > Current '!' aliases move cwd to $GIT_WORK_TREE first, which could make > > handling path arguments hard because they are relative to the original > > cwd. We set GIT_PREFIX to work around it, but I still think it's more > > natural to keep cwd where it is. > > > > We have a way to do that now after 441981b (git: simplify environment > > save/restore logic - 2016-01-26). It's just a matter of choosing the > > right syntax. I'm going with '!!'. I'm not very happy with it. But I > > do like this type of alias. > > I do not know why you are not happy with the syntax, but I > personally think it brilliant, both the idea and the preliminary > clean-up that made this possible with a simple patch like this. I guess he is not happy with it because "!!" is quite unintuitive a construct. I know that *I* would have been puzzled by it, asking "What the heck does this do?". Ciao, Dscho
[PATCH] clean up confusing suggestion for commit references
The description for referencing commits looks as if it is contradicting the example, since it is itself enclosed in double quotes. Lets use single quotes around the description and include the double quotes in the description so it matches the example. --- Sorry for opening this up again but I just looked up the format and was like: "Umm, which one is now the correct one..." For this makes more sense. What do others think? Cheers Heiko Documentation/SubmittingPatches | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index 08352de..692f4ce 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -122,7 +122,7 @@ without external resources. Instead of giving a URL to a mailing list archive, summarize the relevant points of the discussion. If you want to reference a previous commit in the history of a stable -branch, use the format "abbreviated sha1 (subject, date)", +branch, use the format 'abbreviated sha1 ("subject", date)', with the subject enclosed in a pair of double-quotes, like this: Commit f86a374 ("pack-bitmap.c: fix a memleak", 2015-03-30) -- 2.10.0.645.g54f1e86