* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Johannes Schindelin @ 2016-10-08 8:36 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Duy Nguyen, Junio C Hamano, Git Mailing List
In-Reply-To: <20161007175052.sxyk7y2ytjh36phr@sigill.intra.peff.net>
Hi Peff & Hannes,
On Fri, 7 Oct 2016, Jeff King wrote:
> 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.
So what about this?
[alias]
d2u = !dos2unix
[alias "d2u"]
shell = 'f() { git ls-files "$@" | xargs dos2unix; }; f'
exec = C:/cygwin64/bin/dos2unix.exe
You introduce all kinds of ambiguities here that did not exist before...
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v4 4/4] mergetool: honor -O<orderfile>
From: Johannes Sixt @ 2016-10-08 7:46 UTC (permalink / raw)
To: David Aguilar; +Cc: Git Mailing List, Junio C Hamano, Luis Gutierrez
In-Reply-To: <20161008000130.22858-1-davvid@gmail.com>
With this final fixup, the series looks good to me, and I have no
further comments.
Reviewed-by: Johannes Sixt <j6t@kdbg.org>
-- Hannes
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Duy Nguyen @ 2016-10-08 0:32 UTC (permalink / raw)
To: Jakub Narębski
Cc: Johannes Schindelin, Junio C Hamano, Git Mailing List,
SZEDER Gábor, Johannes Sixt
In-Reply-To: <f41a1371-7771-1960-27e0-cba0ee5b1461@gmail.com>
On Fri, Oct 7, 2016 at 10:55 PM, Jakub Narębski <jnareb@gmail.com> 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.4ecf3ptl6msztoya@sigill.intra.peff.net/T/#mb1d7b8f31d595b05105b8ea2137756761e13dbf4
--
Duy
^ permalink raw reply
* [PATCH] remote.c: free previous results when looking for a ref match
From: Stefan Beller @ 2016-10-07 23:58 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
Signed-off-by: Stefan Beller <sbeller@google.com>
---
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
^ permalink raw reply related
* [PATCH v4 4/4] mergetool: honor -O<orderfile>
From: David Aguilar @ 2016-10-08 0:01 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Johannes Sixt, Luis Gutierrez
Teach mergetool to pass "-O<orderfile>" down to `git diff` when
specified on the command-line.
Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
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<orderfile>::
+ Process files in the order specified in the
+ <orderfile>, 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<orderfile>] [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
^ permalink raw reply related
* [PATCH v3 4/4] mergetool: honor -O<orderfile>
From: David Aguilar @ 2016-10-07 23:58 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Johannes Sixt, Luis Gutierrez
In-Reply-To: <20161007235806.22247-1-davvid@gmail.com>
Teach mergetool to pass "-O<orderfile>" down to `git diff` when
specified on the command-line.
Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
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<orderfile>::
+ Process files in the order specified in the
+ <orderfile>, 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<orderfile>] [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
^ permalink raw reply related
* [PATCH v3 3/4] mergetool: honor diff.orderFile
From: David Aguilar @ 2016-10-07 23:58 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Johannes Sixt, Luis Gutierrez
In-Reply-To: <20161007235806.22247-1-davvid@gmail.com>
Teach mergetool to get the list of files to edit via `diff` so that we
gain support for diff.orderFile.
Suggested-by: Luis Gutierrez <luisgutz@gmail.com>
Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
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
^ permalink raw reply related
* [PATCH v3 2/4] mergetool: move main program flow into a main() function
From: David Aguilar @ 2016-10-07 23:58 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Johannes Sixt, Luis Gutierrez
In-Reply-To: <20161007235806.22247-1-davvid@gmail.com>
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 <davvid@gmail.com>
---
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 |
+ sed -e 's/^[^ ]* //' | sort -u)
+ fi
else
- files=$(git ls-files -u | sed -e 's/^[^ ]* //' | sort -u)
+ files=$(git ls-files -u -- "$@" |
+ sed -e 's/^[^ ]* //' | sort -u)
fi
-else
- files=$(git ls-files -u -- "$@" | sed -e 's/^[^ ]* //' | sort -u)
-fi
-
-if test -z "$files"
-then
- echo "No files need merging"
- exit 0
-fi
-
-printf "Merging:\n"
-printf "%s\n" "$files"
-
-rc=0
-for i in $files
-do
- printf "\n"
- if ! merge_file "$i"
+
+ if test -z "$files"
then
- rc=1
- prompt_after_failed_merge || exit 1
+ echo "No files need merging"
+ exit 0
fi
-done
-exit $rc
+ printf "Merging:\n"
+ printf "%s\n" "$files"
+
+ rc=0
+ for i in $files
+ do
+ printf "\n"
+ if ! merge_file "$i"
+ then
+ rc=1
+ prompt_after_failed_merge || exit 1
+ fi
+ done
+
+ exit $rc
+}
+
+main "$@"
--
2.10.1.386.g8ee99a0
^ permalink raw reply related
* [PATCH v3 1/4] mergetool: add copyright
From: David Aguilar @ 2016-10-07 23:58 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Johannes Sixt, Luis Gutierrez
Signed-off-by: David Aguilar <davvid@gmail.com>
---
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
^ permalink raw reply related
* [PATCH v4 5/7] builtin/tag: add --format argument for tag -v
From: santiago @ 2016-10-07 21:07 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas Puehringer
In-Reply-To: <20161007210721.20437-1-santiago@nyu.edu>
From: Lukas Puehringer <luk.puehringer@gmail.com>
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 <luk.puehringer@gmail.com>
---
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[<num>]] -l [--contains <commit>] [--points-at <object>]
[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]
-'git tag' -v <tagname>...
+'git tag' -v [--format=<format>] <tagname>...
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 <tagname>..."),
N_("git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>]"
"\n\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"),
- N_("git tag -v <tagname>..."),
+ N_("git tag -v [--format=<format>] <tagname>..."),
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
^ permalink raw reply related
* [PATCH v4 7/7] t/t7004-tag: Add --format specifier tests
From: santiago @ 2016-10-07 21:07 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres
In-Reply-To: <20161007210721.20437-1-santiago@nyu.edu>
From: Santiago Torres <santiago@nyu.edu>
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 <santiago@nyu.edu>
---
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
^ permalink raw reply related
* [PATCH v4 6/7] t/t7030-verify-tag: Add --format specifier tests
From: santiago @ 2016-10-07 21:07 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres
In-Reply-To: <20161007210721.20437-1-santiago@nyu.edu>
From: Santiago Torres <santiago@nyu.edu>
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 <santiago@nyu.edu>
---
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
^ permalink raw reply related
* [PATCH v4 1/7] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: santiago @ 2016-10-07 21:07 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas Puehringer
In-Reply-To: <20161007210721.20437-1-santiago@nyu.edu>
From: Lukas Puehringer <luk.puehringer@gmail.com>
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 <luk.puehringer@gmail.com>
---
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
^ permalink raw reply related
* [PATCH v4 3/7] tag: add format specifier to gpg_verify_tag
From: santiago @ 2016-10-07 21:07 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas Puehringer
In-Reply-To: <20161007210721.20437-1-santiago@nyu.edu>
From: Lukas Puehringer <luk.puehringer@gmail.com>
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 <luk.puehringer@gmail.com>
---
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
^ permalink raw reply related
* [PATCH v4 4/7] builtin/verify-tag: add --format to verify-tag
From: santiago @ 2016-10-07 21:07 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres
In-Reply-To: <20161007210721.20437-1-santiago@nyu.edu>
From: Santiago Torres <santiago@nyu.edu>
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 <santiago@nyu.edu>
---
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' <tag>...
+'git verify-tag' [--format=<format>] <tag>...
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 <signal.h>
#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] <tag>..."),
+ N_("git verify-tag [-v | --verbose] [--format=<format>] <tag>..."),
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
^ permalink raw reply related
* [PATCH v4 0/7] Add --format to tag verification
From: santiago @ 2016-10-07 21:07 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres
From: Santiago Torres <santiago@nyu.edu>
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-santiago@nyu.edu/
[2] http://public-inbox.org/git/20160922185317.349-1-santiago@nyu.edu/
[3] http://public-inbox.org/git/20160926224233.32702-1-santiago@nyu.edu/
[4] http://public-inbox.org/git/20160607195608.16643-1-santiago@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
^ permalink raw reply
* [PATCH v4 2/7] ref-filter: add function to print single ref_array_item
From: santiago @ 2016-10-07 21:07 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas Puehringer
In-Reply-To: <20161007210721.20437-1-santiago@nyu.edu>
From: Lukas Puehringer <luk.puehringer@gmail.com>
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 <luk.puehringer@gmail.com>
---
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
^ permalink raw reply related
* Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2
From: René Scharfe @ 2016-10-07 20:45 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano
In-Reply-To: <20161007004635.xbuaxvc6eyfkyyma@sigill.intra.peff.net>
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é
^ permalink raw reply
* RE: Uninitialized submodules as symlinks
From: David Turner @ 2016-10-07 19:59 UTC (permalink / raw)
To: 'Stefan Beller'; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79ka1rJCks5np9Q__HAHYFDrdGDFLtquhb3fY_iL8q9DEkw@mail.gmail.com>
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@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 <David.Turner@twosigma.com>
> 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 <pathspec>
> custom-submodule make-symlink <pathspec>
>
> ?
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/<path>
> git config submodule.<name>.symlinked ~/my/private/symlinked
> # The <name> 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.<name>.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.<name>.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.
^ permalink raw reply
* Re: Uninitialized submodules as symlinks
From: Stefan Beller @ 2016-10-07 18:56 UTC (permalink / raw)
To: David Turner; +Cc: git@vger.kernel.org
In-Reply-To: <fd5bcf57f92944c0b7f6f2f8342c342c@exmbdft7.ad.twosigma.com>
On Fri, Oct 7, 2016 at 11:17 AM, David Turner <David.Turner@twosigma.com> 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 <pathspec>
custom-submodule make-symlink <pathspec>
?
> 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/<path>
git config submodule.<name>.symlinked ~/my/private/symlinked
# The <name> 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.<name>.ignore to suppress status looking
at all files.
When looking through the options, you could add the value "symlink" to
submodule.<name>.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.
^ permalink raw reply
* Re: "Purposes, Concepts,Misfits, and a Redesign of Git" (a research paper)
From: Jakub Narębski @ 2016-10-07 18:56 UTC (permalink / raw)
To: Konstantin Khomoutov; +Cc: git
In-Reply-To: <20161007205220.e7cc582a042f12cd13b6aef7@domain007.com>
W dniu 07.10.2016 o 19:52, Konstantin Khomoutov pisze:
> On Fri, 30 Sep 2016 19:14:13 +0300
> Konstantin Khomoutov <kostix+git@007spb.ru> 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-5ebb0f004eb5@gmail.com/T/#e95875b7940512b432ab2e29b3dd50ca448df9720
Posted. Thanks for the idea.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v7 0/4] recursive support for ls-files
From: Brandon Williams @ 2016-10-07 18:45 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Jeff King, Junio C Hamano
In-Reply-To: <CAGZ79kZo-hL5-rCdw2VA5ce+xpizbGUdkS8ddb6eMC8qQoRSVw@mail.gmail.com>
On 10/07, Stefan Beller wrote:
> On Fri, Oct 7, 2016 at 11:34 AM, Stefan Beller <sbeller@google.com> wrote:
> > On Fri, Oct 7, 2016 at 11:18 AM, Brandon Williams <bmwill@google.com> 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
^ permalink raw reply
* Re: [PATCH v7 0/4] recursive support for ls-files
From: Stefan Beller @ 2016-10-07 18:35 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jeff King, Junio C Hamano
In-Reply-To: <CAGZ79kacgsPg-dzx7JRyhA2bq9JMb2Cyg9PjWbnKi9ZMTPw7RQ@mail.gmail.com>
On Fri, Oct 7, 2016 at 11:34 AM, Stefan Beller <sbeller@google.com> wrote:
> On Fri, Oct 7, 2016 at 11:18 AM, Brandon Williams <bmwill@google.com> wrote:
>> Few minor fixes pointed out Stefan
>
The series itself looks good though. :)
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v7 0/4] recursive support for ls-files
From: Stefan Beller @ 2016-10-07 18:34 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Jeff King, Junio C Hamano
In-Reply-To: <20161007181851.15063-1-bmwill@google.com>
On Fri, Oct 7, 2016 at 11:18 AM, Brandon Williams <bmwill@google.com> 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/<what-junio-queued>)
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
^ permalink raw reply
* Re: [PATCH v2 3/3] batch check whether submodule needs pushing into one call
From: Stefan Beller @ 2016-10-07 18:30 UTC (permalink / raw)
To: Heiko Voigt
Cc: Junio C Hamano, Jeff King, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <67d4c48dc0129f20041c88d27a49c7a21188c882.1475851621.git.hvoigt@hvoigt.net>
On Fri, Oct 7, 2016 at 8:06 AM, Heiko Voigt <hvoigt@hvoigt.net> 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 <hvoigt@hvoigt.net>
> ---
> 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
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox