* Re: Can git choose perl at runtime?
From: Ævar Arnfjörð Bjarmason @ 2018-12-19 13:53 UTC (permalink / raw)
To: Carlo Arenas; +Cc: John Passaro, git
In-Reply-To: <CAPUEspgw2xYxNQN-0_nqQrWE4jhmMN-9aHgJ8NvLDcEKTrZNAw@mail.gmail.com>
On Wed, Dec 19 2018, Carlo Arenas wrote:
> On Tue, Dec 18, 2018 at 7:29 PM John Passaro <john.a.passaro@gmail.com> wrote:
>>
>> I recently submitted my first patch using OSX and found the experience
>> frustrating, for reasons that have come up on the list before,
>> concerning git-send-email and perl dependencies that you need to be
>> root to update.
>
> you can install them somewhere else (your homedir, for example) and
> then instruct perl to look for them there by setting the PERL5LIB
> environment variable
Note that this is different. Cases I can think of:
1. You have an entirely different perl + modules somewhere. E.g. maybe
on OSX /usr/bin/perl v.s. some homebrew version of perl+CPAN. My WIP
https://public-inbox.org/git/87a7l1fx8x.fsf@evledraar.gmail.com/
addresses this.
2. You're happy with /usr/bin/perl (or whatever git is compiled with),
but miss some module(s). That's your suggestion here, but note that
in this case you usually need a compiler (E-Mail SSL libs etc.),
which may not be what the user wants.
I.e. they can install a new perl+modules from their package manager
easily, but can't as easily build their own modules for a system
perl.
3. Using a /usr/bin/perl + e.g. homebrew CPAN libs via a "modules over
here" facility similar to #2 is likely to segfault (different ABI
versions).
I think we're good if we just have #1 and if people have the #2 use-case
an additional core.perlLibs config of stuff to prepend to @INC (or maybe
entirly override, least we run into the segfault case in #3).
For that last bit see also 7a7bfc7adc ("perl: treat PERLLIB_EXTRA as an
extra path again", 2018-01-02). I.e. there's the use case of "@INC
instead of" and "@INC extra".
But probably you're happy with just #1 for now....
^ permalink raw reply
* Re: Can git choose perl at runtime?
From: Ævar Arnfjörð Bjarmason @ 2018-12-19 13:43 UTC (permalink / raw)
To: John Passaro; +Cc: git
In-Reply-To: <CAJdN7Kioa22xrDP2ssZXmBbu7KDkcr2MQCUDW=Tzm5ydzeChBQ@mail.gmail.com>
On Wed, Dec 19 2018, John Passaro wrote:
> I recently submitted my first patch using OSX and found the experience
> frustrating, for reasons that have come up on the list before,
> concerning git-send-email and perl dependencies that you need to be
> root to update.
>
> Last seen here:
> https://public-inbox.org/git/878t55qga6.fsf@evledraar.gmail.com/
>
> The struggle is that Mac's package manager Homebrew has opted,
> apparently with some finality, to no longer support linking to a user
> perl at build time. PERL_PATH is hard-coded to link to the system
> perl, which means the user needs sudo to install the SSL libraries
> required for send-email. So for send-email to work, you need to either
> sudo cpan or build git yourself. The obvious solution here would be to
> do /usr/bin/env perl, but in the above message Aevar pointed out
> pitfalls with that.
>
> It seems that choosing perl at compile time necessarily comes with
> tradeoffs. So I wonder if there is a way we can support choosing a
> perl at runtime without breaking the existing mechanism of linking to
> perl at compile time.
>
> I'm picturing adding an executable "git-perl" to libexec that checks
> config core.perlPath and envvar GIT_PERL_PATH, in some order. Having
> chosen one of these or the build-time PERL_PATH as a last resort, it
> exec's the correct perl executable.
>
> Then relevant scripts (e.g. git-add--interactive, git-send-email)
> invoke git-perl instead of /usr/bin/perl, and the makefile no longer
> replaces that with PERL_PATH -- instead that will be used at runtime
> via git-perl when we can be sure the user does not explicitly prefer
> something different.
>
> That does mean we have a new command to support and document: "git
> perl". If it is preferred to keep this hidden as an implementation
> detail, we could call the executable something like "util-git-perl"
> instead so that it doesn't show up when scanning libexec for git
> commands.
>
> Does this seem like a good idea? I'd be happy to work on a patch.
I see no problem with this. As I noted in my message you linked to doing
this unconditionally is a bad idea, but we can just do it with a config,
e.g. this works:
diff --git a/perl/header_templates/fixed_prefix.template.pl b/perl/header_templates/fixed_prefix.template.pl
index 857b4391a4..f96e2ecd11 100644
--- a/perl/header_templates/fixed_prefix.template.pl
+++ b/perl/header_templates/fixed_prefix.template.pl
@@ -1 +1,7 @@
+BEGIN {
+ chomp(my $perlPath = `git config --get core.perlPath`);;
+ if ($perlPath and $^X ne $perlPath) {
+ exec($perlPath, $0, @ARGV);
+ }
+}
use lib (split(/@@PATHSEP@@/, $ENV{GITPERLLIB} || '@@INSTLIBDIR@@'));
Here you just optionally set core.perlPath in your config and if set
it'll chainload to the new interpreter you point at.
I leave wondering if you also want a setting for @INC there, dealing
with perl/header_templates/runtime_prefix.template.pl and docs/tests as
an exercise for the reader :)
^ permalink raw reply
* [PATCH v2 3/3] pack-redundant: remove unused functions
From: Jiang Xin @ 2018-12-19 12:14 UTC (permalink / raw)
To: Git List; +Cc: Sun Chao, Jiang Xin
In-Reply-To: <20181218095829.20092-2-worldhello.net@gmail.com>
From: Sun Chao <sunchao9@huawei.com>
Remove unused functions to find `min` packs, such as `get_permutations`,
`pll_free`, etc.
Signed-off-by: Sun Chao <sunchao9@huawei.com>
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
builtin/pack-redundant.c | 72 ----------------------------------------
1 file changed, 72 deletions(-)
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 3655cc7dc6..9630117c90 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -285,78 +285,6 @@ static void cmp_two_packs(struct pack_list *p1, struct pack_list *p2)
}
}
-static void pll_free(struct pll *l)
-{
- struct pll *old;
- struct pack_list *opl;
-
- while (l) {
- old = l;
- while (l->pl) {
- opl = l->pl;
- l->pl = opl->next;
- free(opl);
- }
- l = l->next;
- free(old);
- }
-}
-
-/* all the permutations have to be free()d at the same time,
- * since they refer to each other
- */
-static struct pll * get_permutations(struct pack_list *list, int n)
-{
- struct pll *subset, *ret = NULL, *new_pll = NULL;
-
- if (list == NULL || pack_list_size(list) < n || n == 0)
- return NULL;
-
- if (n == 1) {
- while (list) {
- new_pll = xmalloc(sizeof(*new_pll));
- new_pll->pl = NULL;
- pack_list_insert(&new_pll->pl, list);
- new_pll->next = ret;
- ret = new_pll;
- list = list->next;
- }
- return ret;
- }
-
- while (list->next) {
- subset = get_permutations(list->next, n - 1);
- while (subset) {
- new_pll = xmalloc(sizeof(*new_pll));
- new_pll->pl = subset->pl;
- pack_list_insert(&new_pll->pl, list);
- new_pll->next = ret;
- ret = new_pll;
- subset = subset->next;
- }
- list = list->next;
- }
- return ret;
-}
-
-static int is_superset(struct pack_list *pl, struct llist *list)
-{
- struct llist *diff;
-
- diff = llist_copy(list);
-
- while (pl) {
- llist_sorted_difference_inplace(diff, pl->all_objects);
- if (diff->size == 0) { /* we're done */
- llist_free(diff);
- return 1;
- }
- pl = pl->next;
- }
- llist_free(diff);
- return 0;
-}
-
static size_t sizeof_union(struct packed_git *p1, struct packed_git *p2)
{
size_t ret = 0;
--
2.20.0.3.gc45e608566
^ permalink raw reply related
* [PATCH v2 2/3] pack-redundant: new algorithm to find min packs
From: Jiang Xin @ 2018-12-19 12:14 UTC (permalink / raw)
To: Git List; +Cc: Sun Chao, Jiang Xin
In-Reply-To: <20181218095829.20092-2-worldhello.net@gmail.com>
From: Sun Chao <sunchao9@huawei.com>
When calling `git pack-redundant --all`, if there are too many local
packs and too many redundant objects within them, the too deep iteration
of `get_permutations` will exhaust all the resources, and the process of
`git pack-redundant` will be killed.
The following script could create a repository with too many redundant
packs, and running `git pack-redundant --all` in the `test.git` repo
will die soon.
#!/bin/sh
repo="$(pwd)/test.git"
work="$(pwd)/test"
i=1
max=199
if test -d "$repo" || test -d "$work"; then
echo >&2 "ERROR: '$repo' or '$work' already exist"
exit 1
fi
git init -q --bare "$repo"
git --git-dir="$repo" config gc.auto 0
git --git-dir="$repo" config transfer.unpackLimit 0
git clone -q "$repo" "$work" 2>/dev/null
while :; do
cd "$work"
echo "loop $i: $(date +%s)" >$i
git add $i
git commit -q -sm "loop $i"
git push -q origin HEAD:master
printf "\rCreate pack %4d/%d\t" $i $max
if test $i -ge $max; then break; fi
cd "$repo"
git repack -q
if test $(($i % 2)) -eq 0; then
git repack -aq
pack=$(ls -t $repo/objects/pack/*.pack | head -1)
touch "${pack%.pack}.keep"
fi
i=$((i+1))
done
printf "\ndone\n"
To get the `min` unique pack list, we can replace the iteration in
`minimize` function with a new algorithm, and this could solve this
issue:
1. Get the unique and non_uniqe packs, add the unique packs to the
`min` list.
2. Remove the objects of unique packs from non_unique packs, then each
object left in the non_unique packs will have at least two copies.
3. Sort the non_unique packs by the objects' size, more objects first,
and add the first non_unique pack to `min` list.
4. Drop the duplicated objects from other packs in the ordered
non_unique pack list, and repeat step 3.
Original PR and discussions: https://github.com/jiangxin/git/pull/25
Signed-off-by: Sun Chao <sunchao9@huawei.com>
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
builtin/pack-redundant.c | 109 ++++++++++++++++++++++++---------------
1 file changed, 68 insertions(+), 41 deletions(-)
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index cf9a9aabd4..3655cc7dc6 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -421,14 +421,52 @@ static inline off_t pack_set_bytecount(struct pack_list *pl)
return ret;
}
+static int cmp_pack_list_reverse(const void *a, const void *b)
+{
+ struct pack_list *pl_a = *((struct pack_list **)a);
+ struct pack_list *pl_b = *((struct pack_list **)b);
+ size_t sz_a = pl_a->all_objects->size;
+ size_t sz_b = pl_b->all_objects->size;
+
+ if (sz_a == sz_b)
+ return 0;
+ else if (sz_a < sz_b)
+ return 1;
+ else
+ return -1;
+}
+
+/* Sort pack_list, greater size of all_objects first */
+static void sort_pack_list(struct pack_list **pl)
+{
+ struct pack_list **ary, *p;
+ int i;
+ size_t n = pack_list_size(*pl);
+
+ if (n < 2)
+ return;
+
+ /* prepare an array of packed_list for easier sorting */
+ ary = xcalloc(n, sizeof(struct pack_list *));
+ for (n = 0, p = *pl; p; p = p->next)
+ ary[n++] = p;
+
+ QSORT(ary, n, cmp_pack_list_reverse);
+
+ /* link them back again */
+ for (i = 0; i < n - 1; i++)
+ ary[i]->next = ary[i + 1];
+ ary[n - 1]->next = NULL;
+ *pl = ary[0];
+
+ free(ary);
+}
+
+
static void minimize(struct pack_list **min)
{
- struct pack_list *pl, *unique = NULL,
- *non_unique = NULL, *min_perm = NULL;
- struct pll *perm, *perm_all, *perm_ok = NULL, *new_perm;
- struct llist *missing;
- off_t min_perm_size = 0, perm_size;
- int n;
+ struct pack_list *pl, *unique = NULL, *non_unique = NULL;
+ struct llist *missing, *unique_pack_objects;
pl = local_packs;
while (pl) {
@@ -446,49 +484,37 @@ static void minimize(struct pack_list **min)
pl = pl->next;
}
+ *min = unique;
+
/* return if there are no objects missing from the unique set */
if (missing->size == 0) {
- *min = unique;
free(missing);
return;
}
- /* find the permutations which contain all missing objects */
- for (n = 1; n <= pack_list_size(non_unique) && !perm_ok; n++) {
- perm_all = perm = get_permutations(non_unique, n);
- while (perm) {
- if (is_superset(perm->pl, missing)) {
- new_perm = xmalloc(sizeof(struct pll));
- memcpy(new_perm, perm, sizeof(struct pll));
- new_perm->next = perm_ok;
- perm_ok = new_perm;
- }
- perm = perm->next;
- }
- if (perm_ok)
- break;
- pll_free(perm_all);
- }
- if (perm_ok == NULL)
- die("Internal error: No complete sets found!");
-
- /* find the permutation with the smallest size */
- perm = perm_ok;
- while (perm) {
- perm_size = pack_set_bytecount(perm->pl);
- if (!min_perm_size || min_perm_size > perm_size) {
- min_perm_size = perm_size;
- min_perm = perm->pl;
- }
- perm = perm->next;
- }
- *min = min_perm;
- /* add the unique packs to the list */
- pl = unique;
+ unique_pack_objects = llist_copy(all_objects);
+ llist_sorted_difference_inplace(unique_pack_objects, missing);
+
+ /* remove unique pack objects from the non_unique packs */
+ pl = non_unique;
while (pl) {
- pack_list_insert(min, pl);
+ llist_sorted_difference_inplace(pl->all_objects, unique_pack_objects);
pl = pl->next;
}
+
+ while (non_unique) {
+ /* sort the non_unique packs, greater size of all_objects first */
+ sort_pack_list(&non_unique);
+ if (non_unique->all_objects->size == 0)
+ break;
+
+ pack_list_insert(min, non_unique);
+
+ for (pl = non_unique->next; pl && pl->all_objects->size > 0; pl = pl->next)
+ llist_sorted_difference_inplace(pl->all_objects, non_unique->all_objects);
+
+ non_unique = non_unique->next;
+ }
}
static void load_all_objects(void)
@@ -603,7 +629,7 @@ static void load_all(void)
int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
{
int i;
- struct pack_list *min, *red, *pl;
+ struct pack_list *min = NULL, *red, *pl;
struct llist *ignore;
struct object_id *oid;
char buf[GIT_MAX_HEXSZ + 2]; /* hex hash + \n + \0 */
@@ -664,6 +690,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
pl = local_packs;
while (pl) {
llist_sorted_difference_inplace(pl->unique_objects, ignore);
+ llist_sorted_difference_inplace(pl->all_objects, ignore);
pl = pl->next;
}
--
2.20.0.3.gc45e608566
^ permalink raw reply related
* [PATCH v2 1/3] t5322: test cases for git-pack-redundant
From: Jiang Xin @ 2018-12-19 12:14 UTC (permalink / raw)
To: Git List; +Cc: Jiang Xin, Sun Chao, Jiang Xin
In-Reply-To: <20181218095829.20092-2-worldhello.net@gmail.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Add test cases for git pack-redundant to validate new algorithm for git
pack-redundant.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
t/t5322-pack-redundant.sh | 69 +++++++++++++++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
create mode 100755 t/t5322-pack-redundant.sh
diff --git a/t/t5322-pack-redundant.sh b/t/t5322-pack-redundant.sh
new file mode 100755
index 0000000000..4add9c2bb1
--- /dev/null
+++ b/t/t5322-pack-redundant.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+#
+# Copyright (c) 2018 Jiang Xin
+#
+
+test_description='git redundant test'
+
+. ./test-lib.sh
+
+create_commits()
+{
+ set -e
+ parent=
+ for name in A B C D E F G H I J K L M
+ do
+ test_tick
+ T=$(git write-tree)
+ if test -z "$parent"
+ then
+ sha1=$(echo $name | git commit-tree $T)
+ else
+ sha1=$(echo $name | git commit-tree -p $parent $T)
+ fi
+ eval $name=$sha1
+ parent=$sha1
+ done
+ git update-ref refs/heads/master $M
+}
+
+create_redundant_packs()
+{
+ set -e
+ cd .git/objects/pack
+ P1=$(printf "$T\n$A\n" | git pack-objects pack 2>/dev/null)
+ P2=$(printf "$T\n$A\n$B\n$C\n$D\n$E\n" | git pack-objects pack 2>/dev/null)
+ P3=$(printf "$C\n$D\n$F\n$G\n$I\n$J\n" | git pack-objects pack 2>/dev/null)
+ P4=$(printf "$D\n$E\n$G\n$H\n$J\n$K\n" | git pack-objects pack 2>/dev/null)
+ P5=$(printf "$F\n$G\n$H\n" | git pack-objects pack 2>/dev/null)
+ P6=$(printf "$F\n$I\n$L\n" | git pack-objects pack 2>/dev/null)
+ P7=$(printf "$H\n$K\n$M\n" | git pack-objects pack 2>/dev/null)
+ P8=$(printf "$L\n$M\n" | git pack-objects pack 2>/dev/null)
+ cd -
+}
+
+# Create commits and packs
+create_commits
+create_redundant_packs
+
+test_expect_success 'clear loose objects' '
+ git prune-packed &&
+ test $(find .git/objects -type f | grep -v pack | wc -l) -eq 0
+'
+
+printf "$P1\n$P4\n$P5\n$P6\n" | sort >expected
+
+test_expect_success 'git pack-redundant --all' '
+ git pack-redundant --all | \
+ sed -e "s#^.*/pack-\(.*\)\.\(idx\|pack\)#\1#g" | \
+ sort -u >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'remove redundant packs' '
+ git pack-redundant --all | xargs rm &&
+ git fsck &&
+ test $(git pack-redundant --all | wc -l) -eq 0
+'
+
+test_done
--
2.20.0.3.gc45e608566
^ permalink raw reply related
* [PATCH v2 0/3] pack-redundant: new algorithm to find min packs
From: Jiang Xin @ 2018-12-19 12:14 UTC (permalink / raw)
To: Git List; +Cc: Jiang Xin, Sun Chao
In-Reply-To: <20181218095829.20092-2-worldhello.net@gmail.com>
Sun Chao is my former colleague at Huawei. He finds a bug of git-pack-redundant.
When I was in Huawei, I develop a program to manage fork tree of repositories,
using alternate repo for forks to save disk spaces.
Sun Chao finds if there are too many packs and many of them overlap each
other, running `git pack-redundant --all` will exhaust all memories and the
process will be killed by kernel.
There is a script in commit log of commit 2/3, which can be used to create a
repository with lots of redundant packs. Running `git pack-redundant
--all` in it can reproduce this issue.
Updates of reroll v2:
* Add test cases in t5322.
* Fix a bug in patch 2/3.
--
Jiang Xin (1):
t5322: test cases for git-pack-redundant
Sun Chao (2):
pack-redundant: new algorithm to find min packs
pack-redundant: remove unused functions
builtin/pack-redundant.c | 181 ++++++++++++++------------------------
t/t5322-pack-redundant.sh | 69 +++++++++++++++
2 files changed, 137 insertions(+), 113 deletions(-)
create mode 100755 t/t5322-pack-redundant.sh
--
2.20.0.3.gc45e608566
^ permalink raw reply
* Re: Skipping history and save bandwidth: how can I jump between shallow clones, without sending unused blobs
From: Duy Nguyen @ 2018-12-19 11:30 UTC (permalink / raw)
To: Robin H. Johnson; +Cc: Git Mailing List
In-Reply-To: <robbat2-20181218T203023-954479960Z@orbis-terrarum.net>
On Wed, Dec 19, 2018 at 12:15 AM Robin H. Johnson <robbat2@gentoo.org> wrote:
>
> I think this is encapsulated in the v2/promisor work, but wanted to
> check how close that was to fruition, and that it would indeed be
> possible.
>
> This would enable replacement of any workflow that presently uses rsync
> to update.
>
> If I have a clone (ideally shallow already) at A, I'd like to update it
> to the latest remote tip at F, ALSO at depth 1, without fetching the
> intermediate history (B..E), or blobs unique to the intermediate history
> [and no longer referenced from any tree at F].
>
> I critically want to ensure:
> - that the remote does NOT send any blobs that I already have.
> - that the remote does NOT send any blobs that are not reachable from
> the new tip (e.g. blobs that existed between the old state and the new
> state, but aren't needed anymore).
This is what we do now (I've created a small repo to test it again).
Are you seeing that we transfer more than needed?
>
> --
> Robin Hugh Johnson
> Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
> E-Mail : robbat2@gentoo.org
> GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
> GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136
--
Duy
^ permalink raw reply
* [PATCH] log: add %S option (like --source) to log --format
From: issac.trotts @ 2018-12-19 8:33 UTC (permalink / raw)
To: git; +Cc: noemi, Issac Trotts, Issac Trotts
From: Issac Trotts <issac.trotts@gmail.com>
Make it possible to write for example
git log --format="%H,%S"
where the %S at the end is a new placeholder that prints out the ref
(tag/branch) for each commit.
Using %d might seem like an alternative but it only shows the ref for the last
commit in the branch.
Signed-off-by: Issac Trotts <issactrotts@google.com>
---
This change is based on a question from Stack Overflow:
https://stackoverflow.com/questions/12712775/git-get-source-information-in-format
---
Documentation/pretty-formats.txt | 2 ++
builtin/log.c | 2 +-
log-tree.c | 1 +
pretty.c | 15 ++++++++++
pretty.h | 1 +
t/t4205-log-pretty-formats.sh | 50 ++++++++++++++++++++++++++++++++
6 files changed, 70 insertions(+), 1 deletion(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 417b638cd..de6953108 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -134,6 +134,8 @@ The placeholders are:
- '%cI': committer date, strict ISO 8601 format
- '%d': ref names, like the --decorate option of linkgit:git-log[1]
- '%D': ref names without the " (", ")" wrapping.
+- '%S': ref name given on the command line by which the commit was reached
+ (like `git log --source`), only works with `git log`
- '%e': encoding
- '%s': subject
- '%f': sanitized subject line, suitable for a filename
diff --git a/builtin/log.c b/builtin/log.c
index e8e51068b..be3025657 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -203,7 +203,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
rev->diffopt.filter || rev->diffopt.flags.follow_renames)
rev->always_show_header = 0;
- if (source) {
+ if (source || (rev->pretty_given && (rev->commit_format == CMIT_FMT_USERFORMAT) && w.source)) {
init_revision_sources(&revision_sources);
rev->sources = &revision_sources;
}
diff --git a/log-tree.c b/log-tree.c
index 10680c139..3cb14256e 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -700,6 +700,7 @@ void show_log(struct rev_info *opt)
ctx.color = opt->diffopt.use_color;
ctx.expand_tabs_in_log = opt->expand_tabs_in_log;
ctx.output_encoding = get_log_output_encoding();
+ ctx.rev = opt;
if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
ctx.from_ident = &opt->from_ident;
if (opt->graph)
diff --git a/pretty.c b/pretty.c
index b83a3ecd2..258e86e9c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1084,6 +1084,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
struct commit_list *p;
const char *arg;
int ch;
+ char **slot;
/* these are independent of the commit */
switch (placeholder[0]) {
@@ -1194,6 +1195,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
load_ref_decorations(NULL, DECORATE_SHORT_REFS);
format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
return 1;
+ case 'S': /* tag/branch like --source */
+ if (c->pretty_ctx->rev->sources == NULL) {
+ return 0;
+ }
+ slot = revision_sources_at(c->pretty_ctx->rev->sources, commit);
+ if (slot && *slot) {
+ strbuf_addstr(sb, *slot);
+ return 1;
+ } else {
+ die(_("failed to get info for %%S"));
+ }
case 'g': /* reflog info */
switch(placeholder[1]) {
case 'd': /* reflog selector */
@@ -1498,6 +1510,9 @@ static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,
case 'N':
w->notes = 1;
break;
+ case 'S':
+ w->source = 1;
+ break;
}
return 0;
}
diff --git a/pretty.h b/pretty.h
index 7359d318a..87ca5dfcb 100644
--- a/pretty.h
+++ b/pretty.h
@@ -60,6 +60,7 @@ static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)
struct userformat_want {
unsigned notes:1;
+ unsigned source:1;
};
/* Set the flag "w->notes" if there is placeholder %N in "fmt". */
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 978a8a66f..7df8c3d4e 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -621,4 +621,54 @@ test_expect_success 'trailer parsing not fooled by --- line' '
test_cmp expect actual
'
+test_expect_success 'set up %S tests' '
+ git checkout --orphan source-a &&
+ test_commit one &&
+ test_commit two &&
+ git checkout -b source-b HEAD^ &&
+ test_commit three
+'
+
+test_expect_success 'log --format=%S paints branch names' '
+ cat >expect <<-\EOF &&
+ source-b
+ source-a
+ source-b
+ EOF
+ git log --format=%S source-a source-b >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'log --format=%S paints tag names' '
+ git tag -m tagged source-tag &&
+ cat >expect <<-\EOF &&
+ source-tag
+ source-a
+ source-tag
+ EOF
+ git log --format=%S source-tag source-a >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'log --format=%S paints symmetric ranges' '
+ cat >expect <<-\EOF &&
+ source-b
+ source-a
+ EOF
+ git log --format=%S source-a...source-b >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '%S in git log --format works with other placeholders (part 1)' '
+ git log --format="source-b %h" source-b >expect &&
+ git log --format="%S %h" source-b >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '%S in git log --format works with other placeholders (part 2)' '
+ git log --format="%h source-b" source-b >expect &&
+ git log --format="%h %S" source-b >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.19.1
^ permalink raw reply related
* Re: [PATCH] log: add %S option (like --source) to log --format
From: Issac Trotts @ 2018-12-19 8:07 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Noemi Mercado
In-Reply-To: <CANdyxMzOpRRu+B0cq5cbrmYsy47cod8N4mWrGtfOgRRgKVh+xA@mail.gmail.com>
Travis showed one failure. I'm not sure if it's a flake.
https://travis-ci.org/ijt/git/builds/469843833. Rerunning it.
On Tue, Dec 18, 2018 at 7:47 PM Issac Trotts <issac.trotts@gmail.com> wrote:
>
> On Tue, Dec 18, 2018 at 9:14 AM Issac Trotts <issac.trotts@gmail.com> wrote:
> >
> > Hi Peff, thanks for the feedback. I tried a variant of the command you
> > showed and it yielded a seg fault:
> > ```
> > [ issactrotts ~/git ] ./git diff-tree -s --pretty=tformat:'%S %H %s' HEAD
> > Segmentation fault: 11
> > ```
> > I'll see if I can track it down this evening.
>
> Okay, found it. My solution is to make %S not do anything for commands
> other than `log` since `log` is the only one that has --source defined
> as far as I can tell.
>
> I'm waiting for Travis to run and will post an updated patch if
> everything looks good there.
>
> > Issac
> >
> > On Mon, Dec 17, 2018 at 7:59 AM Jeff King <peff@peff.net> wrote:
> > >
> > > On Sun, Dec 16, 2018 at 10:25:14PM -0800, Issac Trotts wrote:
> > >
> > > > Make it possible to write for example
> > > >
> > > > git log --format="%H,%S"
> > > >
> > > > where the %S at the end is a new placeholder that prints out the ref
> > > > (tag/branch) for each commit.
> > >
> > > Seems like a reasonable thing to want.
> > >
> > > One curious thing about "--source" is that it requires cooperation from
> > > the actual traversal. So if you did:
> > >
> > > git rev-list | git diff-tree --format="%H %S"
> > >
> > > we don't have the %S information in the latter command. I think that's
> > > probably acceptable as long as it does something sane when we don't have
> > > that information (e.g., replace it with an empty string). It might also
> > > be worth calling out in the documentation.
> > >
> > > -Peff
^ permalink raw reply
* bugreport: git checkout --recurse-submodules overwrites unstaged changes
From: Petr Gregor @ 2018-12-19 7:25 UTC (permalink / raw)
To: git
Hello,
I believe we found a reproducible bug in git checkout
--recurse-submodules command. Documentation says, that this command
should not overwrite unstaged changes in submodules unless -f is
given. In reality local changes can be accidentally overwritten even
without -f flag.
I reproduced the issue with git version 2.20.1 like this:
git init
git submodule add https://github.com/Gregy/znapzend-debian submodule
git add . && git commit -m 'first'
git checkout -b 'newbranch'
(cd submodule && git checkout a3a7b0)
git add . && git commit -m 'set new branch to different submodule commit'
echo 'test' > submodule/debian/compat #create an unstaged change
git checkout master --recurse-submodules
#my change is overwritten
Thank you for maintaining git, it is awesome!
Petr Gregor
^ permalink raw reply
* Re: Can git choose perl at runtime?
From: Carlo Arenas @ 2018-12-19 6:33 UTC (permalink / raw)
To: John Passaro; +Cc: git
In-Reply-To: <CAJdN7Kioa22xrDP2ssZXmBbu7KDkcr2MQCUDW=Tzm5ydzeChBQ@mail.gmail.com>
On Tue, Dec 18, 2018 at 7:29 PM John Passaro <john.a.passaro@gmail.com> wrote:
>
> I recently submitted my first patch using OSX and found the experience
> frustrating, for reasons that have come up on the list before,
> concerning git-send-email and perl dependencies that you need to be
> root to update.
you can install them somewhere else (your homedir, for example) and
then instruct perl to look for them there by setting the PERL5LIB
environment variable
Carlo
^ permalink raw reply
* Re: [PATCH 0/4] Expose gpgsig in pretty-print
From: John Passaro @ 2018-12-19 5:59 UTC (permalink / raw)
To: Jeff King; +Cc: Michał Górny, git, Junio C Hamano, Alexey Shumkin
In-Reply-To: <20181217202406.GA12122@sigill.intra.peff.net>
On Fri, Dec 14, 2018 at 6:10 PM John Passaro wrote:
> All seems to work fine when I treat %Gs as a detached signature.
In light of this, my best guess as to why the cleartext PGP message
didn't verify properly is that the commit data normally doesn't end
with \n, but as far as I can tell there's no way to express that in
the cleartext format. I don't see a way around this. However, as long
as the following works, I think we have proof-of-concept that this
enhancement allows you to play with signature data however you please
without leaving it to git under the hood:
gpg --verify <(git show -s --format=format:%Gs commit) <(git show -s
--format=format:%Gp commit)
On Mon, Dec 17, 2018 at 3:24 PM Jeff King <peff@peff.net> wrote:
>
> On Fri, Dec 14, 2018 at 11:07:03AM -0500, John Passaro wrote:
>
> > Then I might rename the other new placeholders too:
> >
> > %Gs: signed commit signature (blank when unsigned)
> > %Gp: signed commit payload (i.e. in practice minus the gpgsig header;
> > also blank when unsigned as well)
>
> One complication: the pretty-printing code sees the commit data in the
> i18n.logOutputEncoding charset (utf8 by default). But the signature will
> be over the raw commit data. That's also utf8 by default, but there may
> be an encoding header indicating that it's something else. In that case,
> you couldn't actually verify the signature from the "%Gs%Gp" pair.
>
> I don't think that's insurmountable in the code. You'll have to jump
> through a few hoops to make sure you have the _original_ payload, but we
> obviously do have that data. However, it does feel a little weird to
> include content from a different encoding in the middle of the log
> output stream which claims to be i18n.logOutputEncoding.
>
Thanks for the feedback! This is an interesting conflict. If the user
requests %Gp, the payload for the signature, they almost certainly do
want it in the original encoding; if i18n.logOutputEncoding is
something incompatible, whether explicitly or by default, that seems
like an error. Not much we can do to reconcile the two requests
(commit encoding vs output encoding) so seems reasonable to treat it
as fatal.
Updated patch coming as soon as I work out Peff's aforementioned "few
hoops" to get properly encoded data -- and also how to test success
and failure!
^ permalink raw reply
* Re: [PATCH] log: add %S option (like --source) to log --format
From: Issac Trotts @ 2018-12-19 3:47 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Noemi Mercado
In-Reply-To: <CANdyxMzsf00k44TVkw+9uL4E3G_0rjYNgiMFK2o7MX83fCMPMQ@mail.gmail.com>
On Tue, Dec 18, 2018 at 9:14 AM Issac Trotts <issac.trotts@gmail.com> wrote:
>
> Hi Peff, thanks for the feedback. I tried a variant of the command you
> showed and it yielded a seg fault:
> ```
> [ issactrotts ~/git ] ./git diff-tree -s --pretty=tformat:'%S %H %s' HEAD
> Segmentation fault: 11
> ```
> I'll see if I can track it down this evening.
Okay, found it. My solution is to make %S not do anything for commands
other than `log` since `log` is the only one that has --source defined
as far as I can tell.
I'm waiting for Travis to run and will post an updated patch if
everything looks good there.
> Issac
>
> On Mon, Dec 17, 2018 at 7:59 AM Jeff King <peff@peff.net> wrote:
> >
> > On Sun, Dec 16, 2018 at 10:25:14PM -0800, Issac Trotts wrote:
> >
> > > Make it possible to write for example
> > >
> > > git log --format="%H,%S"
> > >
> > > where the %S at the end is a new placeholder that prints out the ref
> > > (tag/branch) for each commit.
> >
> > Seems like a reasonable thing to want.
> >
> > One curious thing about "--source" is that it requires cooperation from
> > the actual traversal. So if you did:
> >
> > git rev-list | git diff-tree --format="%H %S"
> >
> > we don't have the %S information in the latter command. I think that's
> > probably acceptable as long as it does something sane when we don't have
> > that information (e.g., replace it with an empty string). It might also
> > be worth calling out in the documentation.
> >
> > -Peff
^ permalink raw reply
* Re: Can git choose perl at runtime?
From: Jonathan Nieder @ 2018-12-19 3:17 UTC (permalink / raw)
To: John Passaro; +Cc: git, Ævar Arnfjörð Bjarmason
In-Reply-To: <CAJdN7Kioa22xrDP2ssZXmBbu7KDkcr2MQCUDW=Tzm5ydzeChBQ@mail.gmail.com>
Hi John,
John Passaro wrote:
> https://public-inbox.org/git/878t55qga6.fsf@evledraar.gmail.com/
>
> The struggle is that Mac's package manager Homebrew has opted,
> apparently with some finality, to no longer support linking to a user
> perl at build time. PERL_PATH is hard-coded to link to the system
> perl, which means the user needs sudo to install the SSL libraries
> required for send-email. So for send-email to work, you need to either
> sudo cpan or build git yourself. The obvious solution here would be to
> do /usr/bin/env perl, but in the above message Aevar pointed out
> pitfalls with that.
>
> It seems that choosing perl at compile time necessarily comes with
> tradeoffs. So I wonder if there is a way we can support choosing a
> perl at runtime without breaking the existing mechanism of linking to
> perl at compile time.
I haven't carefully looked at your exact proposal, but I just wanted
to offer you my support: yes, I would love to see some solution.
Thanks for looking into it.
It would let me remove this bit of horror from my local build script:
APIVER_EXPR='@{[sub{use Config; $$Config{api_version}}->()]}'
XCODE_PERL="/Applications/Xcode.app/Contents/Developer/Library/Perl/5.$APIVER_EXPR/darwin-thread-multi-2level"
make ... PERLLIB_EXTRA="$XCODE_PERL"
(My apologies.)
[...]
> That does mean we have a new command to support and document: "git
> perl". If it is preferred to keep this hidden as an implementation
> detail, we could call the executable something like "util-git-perl"
> instead so that it doesn't show up when scanning libexec for git
> commands.
Typically we handle this kind of thing by putting a double-dash in
the command name. See git-sh--setup, for example.
Thanks and hope that helps,
Jonathan
^ permalink raw reply
* Can git choose perl at runtime?
From: John Passaro @ 2018-12-19 3:09 UTC (permalink / raw)
To: git
I recently submitted my first patch using OSX and found the experience
frustrating, for reasons that have come up on the list before,
concerning git-send-email and perl dependencies that you need to be
root to update.
Last seen here:
https://public-inbox.org/git/878t55qga6.fsf@evledraar.gmail.com/
The struggle is that Mac's package manager Homebrew has opted,
apparently with some finality, to no longer support linking to a user
perl at build time. PERL_PATH is hard-coded to link to the system
perl, which means the user needs sudo to install the SSL libraries
required for send-email. So for send-email to work, you need to either
sudo cpan or build git yourself. The obvious solution here would be to
do /usr/bin/env perl, but in the above message Aevar pointed out
pitfalls with that.
It seems that choosing perl at compile time necessarily comes with
tradeoffs. So I wonder if there is a way we can support choosing a
perl at runtime without breaking the existing mechanism of linking to
perl at compile time.
I'm picturing adding an executable "git-perl" to libexec that checks
config core.perlPath and envvar GIT_PERL_PATH, in some order. Having
chosen one of these or the build-time PERL_PATH as a last resort, it
exec's the correct perl executable.
Then relevant scripts (e.g. git-add--interactive, git-send-email)
invoke git-perl instead of /usr/bin/perl, and the makefile no longer
replaces that with PERL_PATH -- instead that will be used at runtime
via git-perl when we can be sure the user does not explicitly prefer
something different.
That does mean we have a new command to support and document: "git
perl". If it is preferred to keep this hidden as an implementation
detail, we could call the executable something like "util-git-perl"
instead so that it doesn't show up when scanning libexec for git
commands.
Does this seem like a good idea? I'd be happy to work on a patch.
^ permalink raw reply
* error: Use of uninitialized value $hash in chomp
From: Andrew Shearer @ 2018-12-19 2:42 UTC (permalink / raw)
To: git@vger.kernel.org
Hello
I am using a "git svn clone" command to extract our project history from svn into git.
About 30m into the process it fails with:
r50739 = 2a1491de1353b1e3cce50d8f9d383407218a44f1 (refs/remotes/git-svn)
fatal: Cannot open '.git/Git_svn_delta_33316_0_UkxiJV': Permission denied
Use of uninitialized value $hash in chomp at C:/Program Files/Git/mingw64/share/perl5/Git.pm line 929, <GEN11> line 36311.
hash-object -w --stdin-paths --no-filters: command returned error: 128
error closing pipe: Bad file descriptor at C:/Program Files/Git/mingw64/libexec/git-core\git-svn line 0.
error closing pipe: Bad file descriptor at C:/Program Files/Git/mingw64/libexec/git-core\git-svn line 0.
(in cleanup) at /usr/share/perl5/vendor_perl/Error.pm line 198 during global destruction.
I tried updating to the latest build, 2.20.1.windows, but it still fails.
There is nothing particularly special about svn changeset 50739 that I can see compared to any other.
Anyone know why this might be failing or how I could resolve it?
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH 2/3] setup: do not use invalid `repository_format`
From: brian m. carlson @ 2018-12-19 0:18 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, Jeff King
In-Reply-To: <20181218072528.3870492-3-martin.agren@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]
On Tue, Dec 18, 2018 at 08:25:27AM +0100, Martin Ågren wrote:
> I fully admit to not understanding all of this setup code, neither in
> its current incarnation, nor in terms of an ideal end game. This check
> seems like a good thing to do though.
It's definitely complex.
> diff --git a/setup.c b/setup.c
> index 27747af7a3..52c3c9d31f 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1138,7 +1138,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
> gitdir = DEFAULT_GIT_DIR_ENVIRONMENT;
> setup_git_env(gitdir);
> }
> - if (startup_info->have_repository)
> + if (startup_info->have_repository && repo_fmt.version > -1)
> repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
> }
I think this change is fine, because we initialize the value in
the_repository elsewhere, and if there's no repository, this should
never have a value other than the default anyway.
I looked at the other patches in the series and thought they looked sane
as well.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH v5 1/1] protocol: advertise multiple supported versions
From: Josh Steadmon @ 2018-12-18 23:05 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, gitster, sbeller, jonathantanmy, szeder.dev,
Johannes Schindelin, Jonathan Nieder, Jeff King
In-Reply-To: <87h8ff20ol.fsf@evledraar.gmail.com>
On 2018.12.14 23:39, Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, Dec 14 2018, Josh Steadmon wrote:
>
> > On 2018.12.14 21:20, Ævar Arnfjörð Bjarmason wrote:
> >>
> >> On Fri, Nov 16 2018, Josh Steadmon wrote:
> >>
> >> I started looking at this to address
> >> https://public-inbox.org/git/nycvar.QRO.7.76.6.1812141318520.43@tvgsbejvaqbjf.bet/
> >> and was about to send a re-roll of my own series, but then...
> >>
> >> > Currently the client advertises that it supports the wire protocol
> >> > version set in the protocol.version config. However, not all services
> >> > support the same set of protocol versions. For example, git-receive-pack
> >> > supports v1 and v0, but not v2. If a client connects to git-receive-pack
> >> > and requests v2, it will instead be downgraded to v0. Other services,
> >> > such as git-upload-archive, do not do any version negotiation checks.
> >> >
> >> > This patch creates a protocol version registry. Individual client and
> >> > server programs register all the protocol versions they support prior to
> >> > communicating with a remote instance. Versions should be listed in
> >> > preference order; the version specified in protocol.version will
> >> > automatically be moved to the front of the registry.
> >> >
> >> > The protocol version registry is passed to remote helpers via the
> >> > GIT_PROTOCOL environment variable.
> >> >
> >> > Clients now advertise the full list of registered versions. Servers
> >> > select the first allowed version from this advertisement.
> >> >
> >> > Additionally, remove special cases around advertising version=0.
> >> > Previously we avoided adding version negotiation fields in server
> >> > responses if it looked like the client wanted v0. However, including
> >> > these fields does not change behavior, so it's better to have simpler
> >> > code.
> >>
> >> ...this paragraph is new in your v5, from the cover letter: "Changes
> >> from V4: remove special cases around advertising version=0". However as
> >> seen in the code & tests:
> >>
> >> > [...]
> >> > static void push_ssh_options(struct argv_array *args, struct argv_array *env,
> >> > enum ssh_variant variant, const char *port,
> >> > - enum protocol_version version, int flags)
> >> > + const struct strbuf *version_advert, int flags)
> >> > {
> >> > - if (variant == VARIANT_SSH &&
> >> > - version > 0) {
> >> > + if (variant == VARIANT_SSH) {
> >> > argv_array_push(args, "-o");
> >> > argv_array_push(args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT);
> >> > - argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
> >> > - version);
> >> > + argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s",
> >> > + version_advert->buf);
> >> > }
> >> > [...]
> >> > --- a/t/t5601-clone.sh
> >> > +++ b/t/t5601-clone.sh
> >> > @@ -346,7 +346,7 @@ expect_ssh () {
> >> >
> >> > test_expect_success 'clone myhost:src uses ssh' '
> >> > git clone myhost:src ssh-clone &&
> >> > - expect_ssh myhost src
> >> > + expect_ssh "-o SendEnv=GIT_PROTOCOL" myhost src
> >> > '
> >> >
> >> > test_expect_success !MINGW,!CYGWIN 'clone local path foo:bar' '
> >> > @@ -357,12 +357,12 @@ test_expect_success !MINGW,!CYGWIN 'clone local path foo:bar' '
> >> >
> >> > test_expect_success 'bracketed hostnames are still ssh' '
> >> > git clone "[myhost:123]:src" ssh-bracket-clone &&
> >> > - expect_ssh "-p 123" myhost src
> >> > + expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
> >> > '
> >> >
> >> > test_expect_success 'OpenSSH variant passes -4' '
> >> > git clone -4 "[myhost:123]:src" ssh-ipv4-clone &&
> >> > - expect_ssh "-4 -p 123" myhost src
> >> > + expect_ssh "-o SendEnv=GIT_PROTOCOL -4 -p 123" myhost src
> >> > '
> >> >
> >> > test_expect_success 'variant can be overridden' '
> >> > @@ -406,7 +406,7 @@ test_expect_success 'OpenSSH-like uplink is treated as ssh' '
> >> > GIT_SSH="$TRASH_DIRECTORY/uplink" &&
> >> > test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\"" &&
> >> > git clone "[myhost:123]:src" ssh-bracket-clone-sshlike-uplink &&
> >> > - expect_ssh "-p 123" myhost src
> >> > + expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
> >> > '
> >> >
> >> > test_expect_success 'plink is treated specially (as putty)' '
> >> > @@ -446,14 +446,14 @@ test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
> >> > copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
> >> > GIT_SSH_VARIANT=ssh \
> >> > git clone "[myhost:123]:src" ssh-bracket-clone-variant-1 &&
> >> > - expect_ssh "-p 123" myhost src
> >> > + expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
> >> > '
> >> >
> >> > test_expect_success 'ssh.variant overrides plink detection' '
> >> > copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
> >> > git -c ssh.variant=ssh \
> >> > clone "[myhost:123]:src" ssh-bracket-clone-variant-2 &&
> >> > - expect_ssh "-p 123" myhost src
> >> > + expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
> >> > '
> >> >
> >> > test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
> >> > @@ -488,7 +488,7 @@ test_clone_url () {
> >> > }
> >> >
> >> > test_expect_success !MINGW 'clone c:temp is ssl' '
> >> > - test_clone_url c:temp c temp
> >> > + test_clone_url c:temp "-o SendEnv=GIT_PROTOCOL" c temp
> >> > '
> >> >
> >> > test_expect_success MINGW 'clone c:temp is dos drive' '
> >> > @@ -499,7 +499,7 @@ test_expect_success MINGW 'clone c:temp is dos drive' '
> >> > for repo in rep rep/home/project 123
> >> > do
> >> > test_expect_success "clone host:$repo" '
> >> > - test_clone_url host:$repo host $repo
> >> > + test_clone_url host:$repo "-o SendEnv=GIT_PROTOCOL" host $repo
> >> > '
> >> > done
> >> >
> >> > @@ -507,16 +507,16 @@ done
> >> > for repo in rep rep/home/project 123
> >> > do
> >> > test_expect_success "clone [::1]:$repo" '
> >> > - test_clone_url [::1]:$repo ::1 "$repo"
> >> > + test_clone_url [::1]:$repo "-o SendEnv=GIT_PROTOCOL" ::1 "$repo"
> >> > '
> >> > done
> >> > #home directory
> >> > test_expect_success "clone host:/~repo" '
> >> > - test_clone_url host:/~repo host "~repo"
> >> > + test_clone_url host:/~repo "-o SendEnv=GIT_PROTOCOL" host "~repo"
> >> > '
> >> >
> >> > test_expect_success "clone [::1]:/~repo" '
> >> > - test_clone_url [::1]:/~repo ::1 "~repo"
> >> > + test_clone_url [::1]:/~repo "-o SendEnv=GIT_PROTOCOL" ::1 "~repo"
> >> > '
> >> >
> >> > # Corner cases
> >> > @@ -532,22 +532,22 @@ done
> >> > for tcol in "" :
> >> > do
> >> > test_expect_success "clone ssh://host.xz$tcol/home/user/repo" '
> >> > - test_clone_url "ssh://host.xz$tcol/home/user/repo" host.xz /home/user/repo
> >> > + test_clone_url "ssh://host.xz$tcol/home/user/repo" "-o SendEnv=GIT_PROTOCOL" host.xz /home/user/repo
> >> > '
> >> > # from home directory
> >> > test_expect_success "clone ssh://host.xz$tcol/~repo" '
> >> > - test_clone_url "ssh://host.xz$tcol/~repo" host.xz "~repo"
> >> > + test_clone_url "ssh://host.xz$tcol/~repo" "-o SendEnv=GIT_PROTOCOL" host.xz "~repo"
> >> > '
> >> > done
> >> >
> >> > # with port number
> >> > test_expect_success 'clone ssh://host.xz:22/home/user/repo' '
> >> > - test_clone_url "ssh://host.xz:22/home/user/repo" "-p 22 host.xz" "/home/user/repo"
> >> > + test_clone_url "ssh://host.xz:22/home/user/repo" "-o SendEnv=GIT_PROTOCOL -p 22 host.xz" "/home/user/repo"
> >> > '
> >> >
> >> > # from home directory with port number
> >> > test_expect_success 'clone ssh://host.xz:22/~repo' '
> >> > - test_clone_url "ssh://host.xz:22/~repo" "-p 22 host.xz" "~repo"
> >> > + test_clone_url "ssh://host.xz:22/~repo" "-o SendEnv=GIT_PROTOCOL -p 22 host.xz" "~repo"
> >> > '
> >> >
> >> > #IPv6
> >> > @@ -555,7 +555,7 @@ for tuah in ::1 [::1] [::1]: user@::1 user@[::1] user@[::1]: [user@::1] [user@::
> >> > do
> >> > ehost=$(echo $tuah | sed -e "s/1]:/1]/" | tr -d "[]")
> >> > test_expect_success "clone ssh://$tuah/home/user/repo" "
> >> > - test_clone_url ssh://$tuah/home/user/repo $ehost /home/user/repo
> >> > + test_clone_url ssh://$tuah/home/user/repo '-o SendEnv=GIT_PROTOCOL' $ehost /home/user/repo
> >> > "
> >> > done
> >> >
> >> > @@ -564,7 +564,7 @@ for tuah in ::1 [::1] user@::1 user@[::1] [user@::1]
> >> > do
> >> > euah=$(echo $tuah | tr -d "[]")
> >> > test_expect_success "clone ssh://$tuah/~repo" "
> >> > - test_clone_url ssh://$tuah/~repo $euah '~repo'
> >> > + test_clone_url ssh://$tuah/~repo '-o SendEnv=GIT_PROTOCOL' $euah '~repo'
> >> > "
> >> > done
> >> >
> >> > @@ -573,7 +573,7 @@ for tuah in [::1] user@[::1] [user@::1]
> >> > do
> >> > euah=$(echo $tuah | tr -d "[]")
> >> > test_expect_success "clone ssh://$tuah:22/home/user/repo" "
> >> > - test_clone_url ssh://$tuah:22/home/user/repo '-p 22' $euah /home/user/repo
> >> > + test_clone_url ssh://$tuah:22/home/user/repo '-o SendEnv=GIT_PROTOCOL -p 22' $euah /home/user/repo
> >> > "
> >> > done
> >> >
> >> > @@ -582,7 +582,7 @@ for tuah in [::1] user@[::1] [user@::1]
> >> > do
> >> > euah=$(echo $tuah | tr -d "[]")
> >> > test_expect_success "clone ssh://$tuah:22/~repo" "
> >> > - test_clone_url ssh://$tuah:22/~repo '-p 22' $euah '~repo'
> >> > + test_clone_url ssh://$tuah:22/~repo '-o SendEnv=GIT_PROTOCOL -p 22' $euah '~repo'
> >> > "
> >> > done
> >>
> >> ...so now we're unconditionally going to SendEnv=GIT_PROTOCOL to "ssh"
> >> invocations. I don't have an issue with this, but given the change in
> >> the commit message this seems to have snuck under the radar. You're just
> >> talking about always including the version in server responses, nothing
> >> about the client always needing SendEnv=GIT_PROTOCOL now even with v0.
> >
> > Ack. I'll make sure that V6 calls this out in the commit message.
> >
> >
> >> If the server always sends the version now, why don't you need to amend
> >> the same t5400-send-pack.sh tests as in my "tests: mark & fix tests
> >> broken under GIT_TEST_PROTOCOL_VERSION=1"? There's one that spews out
> >> "version" there under my GIT_TEST_PROTOCOL_VERSION=1.
> >
> > Sorry if I'm being dense, but can you point out more specifically what
> > is wrong here? I don't see anything in t5400 that would be affected by
> > this; the final test case ("receive-pack de-dupes .have lines") is the
> > only one that does any tracing, and it strips out everything that's not
> > a ref advertisement before checking the output. Sorry if I'm missing
> > something obvious.
>
> I think I'm just misunderstanding this bit:
>
> Additionally, remove special cases around advertising version=0.
> Previously we avoided adding version negotiation fields in server
> responses if it looked like the client wanted v0. However, including
> these fields does not change behavior, so it's better to have
> simpler code.
Ah yes, that is a bad description, which I will fix in V6. It should
replace "server responses" to instead be "client requests". Sorry for
the confusion, that was a really silly mistake for me to make.
Basically, in the current master, clients will not add a version
advertisement to their request if their only supported version is 0.
With this patch, they will always include a version advertisement.
> I meant that if you pick the series I have up to "tests: add a special
> setup that sets protocol.version", which is at c6f33984fc in a WIP
> branch in github.com/avar/git.git and run:
>
> $ GIT_TEST_PROTOCOL_VERSION=1 ./t5400-send-pack.sh -v -i -x
>
> It'll fail with:
>
> + test_cmp expect refs
> + diff -u expect refs
> --- expect 2018-12-14 22:26:52.485411992 +0000
> +++ refs 2018-12-14 22:26:52.713412148 +0000
> @@ -1,3 +1,4 @@
> +version 1
> 5285e1710002a06a379056b0d21357a999f3c642 refs/heads/master
> 5285e1710002a06a379056b0d21357a999f3c642 refs/remotes/origin/HEAD
> 5285e1710002a06a379056b0d21357a999f3c642 refs/remotes/origin/master
> error: last command exited with $?=1
> not ok 16 - receive-pack de-dupes .have lines
I believe the reason that my series doesn't fail on this test while
yours does with GIT_TEST_PROTOCOL_VERSION=1 is because the "fork" repo
in the test is a clone from the local filesystem. So in my case the
version negotiation differences are not visible in the trace output of
"git push" (since the advert should be in an environment variable in
this case).
> And I read your commit message to mean "v0 clients also support v1
> responses with the version in them, so let's just always send it". But I
> think this is my confusion (but I still don't know what it *really*
> means).
Yeah again, a dumb mistake in the description on my part. We are only
changing whether the client sends a version advertisement. If everything
else is equal, there should be no behavior changes apart from the
version advertisement.
> >> I was worried about this breaking GIT_SSH_COMMAND, but then I see due to
> >> an interaction with picking "what ssh implementation?" we don't pass "-G
> >> -o SendEnv=GIT_PROTOCOL" at all when I have a GIT_SSH_COMMAND, but *do*
> >> pass it to my normal /usr/bin/ssh. Is this intended? Now if I have a
> >> GIT_SSH_COMMAND that expects to wrap openssh I need to pass "-c
> >> ssh.variant=ssh", because "-c ssh.variant=auto" will now omit these new
> >> arguments.
> >
> > I am not an expert on this part of the code, but it seems to be
> > intended. Later on in the function, there are BUG()s that state that
> > VARIANT_AUTO should not be passed to the push_ssh_options() function.
> > And this only changes the behavior when version=0. For protocol versions
> > 1 & 2, VARIANT_AUTO never had SendEnv=GIT_PROTOCOL added to the command
> > line. Again, sorry if I'm missing some implication here.
>
> I'm wondering if we need to worry about some new odd interactions
> between client/servers when using GIT_SSH* wrappers, maybe not, but
> e.g. as opposed to 0da0e49ba1 ("ssh: 'auto' variant to select between
> 'ssh' and 'simple'", 2017-11-20) which noted a similar change had been
> tested at Google internally (and I read it as Google using GIT_SSH* on
> workstations) your commit doesn't make any mention of this case being
> tested / considered.
>
> So do we need to worry about some new interaction here? Maybe not. Just
> something for people more experienced with the ssh integration to chime
> in on.
After taking a look at 0da0e49ba1 (thanks for the find) and reading
fill_ssh_args(), it seems to me that this is the intended behavior.
ssh.variant=auto means that we will run an initial "$ssh_command -G" to
see if the wrapper seems to handle OpenSSH arguments; if so we will
switch the variant to VARIANT_SSH and run again with the proper options
(including SendEnv=GIT_PROTOCOL).
^ permalink raw reply
* Re: Git hooks don't run while commiting in worktree via git-gui
From: Johannes Schindelin @ 2018-12-18 21:56 UTC (permalink / raw)
To: Иван Могиш
Cc: git,
Дмитрий Яковлев
In-Reply-To: <CACu3VMWJ_otp2D6Tu_2adq=J2wcj0nsYhsW5oWxmbidb3afuyg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]
Hi Ivan,
On Tue, 18 Dec 2018, Иван Могиш wrote:
> Hello.
> There is a little difference in behavior when you are committing to
> the worktree from CLI or some git client (tortoisegit/sourcetree) and
> embedded git gui.
>
> If you configure git hooks in your repository and then add a worktree
> (via git worktree add), hooks from main repository works well both in
> main directory and in worktree, if you are using CLI/third-party GUI.
> Committing in the main directory via embedded git-gui works fine too,
> hooks are running. But when you try to commit in the worktree
> directory from git-gui, hooks don't work.
>
> I think I've found the cause of this:
> https://github.com/git/git/blob/master/git-gui/lib/commit.tcl#L238
> variable fd equals {} because proc githook_read calls proc gitdir to
> determine path to hooks.
> https://github.com/git/git/blob/master/git-gui/git-gui.sh#L626
> This proc use variable _gitdir for calculating result. This var equals
> the result of executing git rev-parse --git-dir
> https://github.com/git/git/blob/master/git-gui/git-gui.sh#L1245
> So, the path to hooks for worktree is
> path_to_main_repo/.git/worktrees/my_worktree/hooks, but there are no
> hooks. Hooks are in path_to_main_repo/.git/hooks, from where they are
> correctly (or not?) executed by git cli, while running from worktree
> directory.
>
> If we put hooks to path_to_main_repo/.git/worktrees/my_worktree/hooks
> too, they will work both in git citool and CLI/third-party GUI. But
> they will execute different files, and it may cause some problems.
Sounds like you need https://github.com/git-for-windows/git/pull/1757
(we do not currently have a responsive maintainer for Git GUI,
unfortunately, otherwise this patch would have made it into an official
Git version already).
That patch also has the benefit of heeding core.hooksPath.
Ciao,
Johannes
> --
> Ivan Mogish
> Support Engineer
> Phone: +9115212057
>
^ permalink raw reply
* Skipping history and save bandwidth: how can I jump between shallow clones, without sending unused blobs
From: Robin H. Johnson @ 2018-12-18 21:56 UTC (permalink / raw)
To: Git Mailing List
[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]
I think this is encapsulated in the v2/promisor work, but wanted to
check how close that was to fruition, and that it would indeed be
possible.
This would enable replacement of any workflow that presently uses rsync
to update.
If I have a clone (ideally shallow already) at A, I'd like to update it
to the latest remote tip at F, ALSO at depth 1, without fetching the
intermediate history (B..E), or blobs unique to the intermediate history
[and no longer referenced from any tree at F].
I critically want to ensure:
- that the remote does NOT send any blobs that I already have.
- that the remote does NOT send any blobs that are not reachable from
the new tip (e.g. blobs that existed between the old state and the new
state, but aren't needed anymore).
--
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 1113 bytes --]
^ permalink raw reply
* Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value
From: Anders Waldenborg @ 2018-12-18 21:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Olga Telezhnaya
In-Reply-To: <xmqqa7ldkbwr.fsf@gitster-ct.c.googlers.com>
Junio C Hamano writes:
> That way, we can handle %(trailers:only=bogo) more sensibly,
> no? Syntactically we can recognize that the user wanted to give
> 'bogo' as the value to 'only', and say "'bogo' is not a boolean" if
> we did so.
I agree that proper error reporting for the pretty formatting strings
would be great. But that would depart from the current extremely crude
error handling where incorrect formatting placeholders are just left
unexpanded. How would such change in error handling be done safely, wrt
backwards compatibility changes?
To get good diagnostics for incorrect formatting strings I think the way
forward is to have the formatting strings parsed once into some kind of
AST or machine (as also mentioned by Jeff) that is just executed many
times, instead of parsed each time like today.
anders
^ permalink raw reply
* [PATCH] upload-pack: teach deepen-relative in protocol v2
From: Jonathan Tan @ 2018-12-18 21:24 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, avarab
Commit 685fbd3291 ("fetch-pack: perform a fetch using v2", 2018-03-15)
attempted to teach Git deepen-relative in protocol v2 (among other
things), but it didn't work:
(1) fetch-pack.c needs to emit "deepen-relative".
(2) upload-pack.c needs to ensure that the correct deepen_relative
variable is passed to deepen() (there are two - the static variable
and the one in struct upload_pack_data).
(3) Before deepen() computes the list of reachable shallows, it first
needs to mark all "our" refs as OUR_REF. v2 currently does not do
this, because unlike v0, it is not needed otherwise.
Fix all this and include a test demonstrating that it works now. For
(2), the static variable deepen_relative is also eliminated, removing a
source of confusion.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Similar to my first fix [1], another fix for an issue discovered by
Aevar's GIT_TEST_PROTOCOL_VERSION patches. This one is more
straightforward.
With this fix and my first fix [1], t5500 no longer reveals any more
bugs. (There might be more in other test files.)
[1] https://public-inbox.org/git/20181218010811.143608-1-jonathantanmy@google.com/
---
fetch-pack.c | 2 ++
t/t5702-protocol-v2.sh | 29 +++++++++++++++++++++++++++++
upload-pack.c | 17 +++++++++++++++--
3 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 9691046e64..c383ea46b3 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1042,6 +1042,8 @@ static void add_shallow_requests(struct strbuf *req_buf,
packet_buf_write(req_buf, "deepen-not %s", s->string);
}
}
+ if (args->deepen_relative)
+ packet_buf_write(req_buf, "deepen-relative\n");
}
static void add_wants(int no_dependents, const struct ref *wants, struct strbuf *req_buf)
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8..340953f01c 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -471,6 +471,35 @@ test_expect_success 'upload-pack respects client shallows' '
grep "fetch< version 2" trace
'
+test_expect_success 'deepen-relative' '
+ rm -rf server client trace &&
+
+ test_create_repo server &&
+ test_commit -C server one &&
+ test_commit -C server two &&
+ test_commit -C server three &&
+ git clone --depth 1 "file://$(pwd)/server" client &&
+ test_commit -C server four &&
+
+ # Sanity check that only "three" is downloaded
+ git -C client log --pretty=tformat:%s master >actual &&
+ echo three >expected &&
+ test_cmp expected actual &&
+
+ GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
+ fetch --deepen=1 origin &&
+ # Ensure that protocol v2 is used
+ grep "fetch< version 2" trace &&
+
+ git -C client log --pretty=tformat:%s origin/master >actual &&
+ cat >expected <<-\EOF &&
+ four
+ three
+ two
+ EOF
+ test_cmp expected actual
+'
+
# Test protocol v2 with 'http://' transport
#
. "$TEST_DIRECTORY"/lib-httpd.sh
diff --git a/upload-pack.c b/upload-pack.c
index 5e81f1ff24..9df27b55a0 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -43,7 +43,6 @@
static timestamp_t oldest_have;
-static int deepen_relative;
static int multi_ack;
static int no_done;
static int use_thin_pack, use_ofs_delta, use_include_tag;
@@ -662,6 +661,9 @@ static void send_unshallow(const struct object_array *shallows,
}
}
+static int check_ref(const char *refname_full, const struct object_id *oid,
+ int flag, void *cb_data);
+
static void deepen(int depth, int deepen_relative,
struct object_array *shallows, struct object_array *want_obj)
{
@@ -676,6 +678,13 @@ static void deepen(int depth, int deepen_relative,
struct object_array reachable_shallows = OBJECT_ARRAY_INIT;
struct commit_list *result;
+ /*
+ * Checking for reachable shallows requires that our refs be
+ * marked with OUR_REF.
+ */
+ head_ref_namespaced(check_ref, NULL);
+ for_each_namespaced_ref(check_ref, NULL);
+
get_reachable_list(shallows, &reachable_shallows);
result = get_shallow_commits(&reachable_shallows,
depth + 1,
@@ -712,6 +721,7 @@ static void deepen_by_rev_list(int ac, const char **av,
static int send_shallow_list(int depth, int deepen_rev_list,
timestamp_t deepen_since,
struct string_list *deepen_not,
+ int deepen_relative,
struct object_array *shallows,
struct object_array *want_obj)
{
@@ -834,6 +844,7 @@ static void receive_needs(struct object_array *want_obj)
int has_non_tip = 0;
timestamp_t deepen_since = 0;
int deepen_rev_list = 0;
+ int deepen_relative = 0;
shallow_nr = 0;
for (;;) {
@@ -925,7 +936,8 @@ static void receive_needs(struct object_array *want_obj)
return;
if (send_shallow_list(depth, deepen_rev_list, deepen_since,
- &deepen_not, &shallows, want_obj))
+ &deepen_not, deepen_relative, &shallows,
+ want_obj))
packet_flush(1);
object_array_clear(&shallows);
}
@@ -1398,6 +1410,7 @@ static void send_shallow_info(struct upload_pack_data *data,
if (!send_shallow_list(data->depth, data->deepen_rev_list,
data->deepen_since, &data->deepen_not,
+ data->deepen_relative,
&data->shallows, want_obj) &&
is_repository_shallow(the_repository))
deepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows,
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related
* Re: [PATCH v4 0/3] Add commit-graph fuzzer and fix buffer overflow
From: Josh Steadmon @ 2018-12-18 21:05 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, stolee, avarab, szeder.dev
In-Reply-To: <20181218173539.GA31070@sigill.intra.peff.net>
On 2018.12.18 12:35, Jeff King wrote:
> On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote:
>
> > Add a new fuzz test for the commit graph and fix a buffer read-overflow
> > that it discovered. Additionally, fix the Makefile instructions for
> > building fuzzers.
> >
> > Changes since V3:
> > * Improve portability of the new test functionality.
>
> I thought there was some question about /dev/zero, which I think is
> in this version (I don't actually know whether there are portability
> issues or not, but somebody did mention it).
>
> -Peff
I've only found one reference [1] (from 1999) of OS X Server not having
a /dev/zero. It appears to be present as of 2010 though [2].
[1]: https://macosx-admin.omnigroup.narkive.com/sAxj0XeP/dev-zero-equivalent-in-mac-os-x
[2]: https://serverfault.com/questions/143248/zeroing-a-disk-with-dd-vs-disk-utility
^ permalink raw reply
* Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.
From: Erin Dahlgren @ 2018-12-18 20:54 UTC (permalink / raw)
To: Jeff King; +Cc: git, Johannes Schindelin, Junio C Hamano
In-Reply-To: <20181218175418.GB31070@sigill.intra.peff.net>
Hi Peff,
On Tue, Dec 18, 2018 at 9:54 AM Jeff King <peff@peff.net> wrote:
>
> On Sat, Dec 15, 2018 at 05:05:08PM -0800, Erin Dahlgren wrote:
>
> > setup_git_directory_gently() expects two types of failures to
> > discover a git directory (e.g. .git/):
> > [...]
>
> When I read your subject line, I'll admit to having a funny feeling in
> the pit of my stomach. This setup code has historically been full of
> subtle corner cases, and I expected a very tricky review.
>
> However, your explanation was so well-written that it was a pleasure to
> read. :)
Thanks :)
>
> > Before this change are two misleading additional behaviors:
> >
> > - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no
> > apparent reason. We never had the chance to change directories
> > up to this point so chdir(current cwd) is pointless.
>
> That makes sense. I think this is a holdover from when we used to walk
> backwards via chdir(), prior to ce9b8aab5d (setup_git_directory_1():
> avoid changing global state, 2017-03-13). Back then, we needed to
> restore the state at this point, but now we don't.
>
> > - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer
> > of a static struct strbuf (cwd). This is unnecessary because the
> > struct is static so its buffer is always reachable. This is also
> > misleading because nowhere else in the function is this buffer
> > released.
>
> Makes sense.
>
> I do have one question, though:
>
> > case GIT_DIR_HIT_CEILING:
> > - prefix = setup_nongit(cwd.buf, nongit_ok);
> > - break;
> > + if (!nongit_ok)
> > + die(_("not a git repository (or any of the parent directories): %s"),
> >a + DEFAULT_GIT_DIR_ENVIRONMENT);
> > + *nongit_ok = 1;
> > + strbuf_release(&dir);
> > + return NULL;
>
> This case used to break out of the switch, and now returns early.
>
> So we do not execute the later code which clears GIT_PREFIX_ENVIRONMENT,
> and zeroes the fields in startup_info. Those probably don't matter in
> most cases, but I wonder if there are some obscure ones where it might.
>
> Might it make sense to make GIT_DIR_HIT_MOUNT_POINT more like
> GIT_DIR_HIT_CEILING currently is, rather than the other way around?
> I.e., something like:
>
> case GIT_DIR_HIT_CEILING:
> if (!nongit_ok)
> die(...);
> *nongit_ok = 1;
> prefix = NULL;
> break;
> case GIT_DIR_HIT_MOUNT_POINT:
> if (!nongit_ok)
> die(...);
> *nongit_ok = 1;
> prefix = NULL;
> break;
>
> ?
After some digging, there seems to be a documented guarantee that
"`GIT_PREFIX` is set as returned by running 'git rev-parse
--show-prefix'". See Documentation/config/alias.txt. If the
environment variable GIT_PREFIX is already set to something and we
don't clear it when prefix is NULL, then the two can be out of sync.
So that's a problem with my patch and the current handling of
GIT_DIR_HIT_MOUNT_POINT. I'm not sure how important of a guarantee it
is, but we should respect what's documented.
Side note: One of the secondary goals of my patch was to make it
really obvious that neither the GIT_DIR_HIT_CEILING nor the
GIT_DIR_HIT_MOUNT_POINT case can get us into the block protected by
(startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)). With
your suggestion (and it's a fair one) I don't think that's feasible
without more significant refactoring. Should I just settle with a
comment that explains this?
Thanks,
Erin
>
> -Peff
^ permalink raw reply
* Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.
From: Erin Dahlgren @ 2018-12-18 19:50 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <nycvar.QRO.7.76.6.1812181332370.43@tvgsbejvaqbjf.bet>
Sorry Johannes for the repeat message, I failed to send to all.
On Tue, Dec 18, 2018 at 4:35 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Erin,
>
> On Sat, 15 Dec 2018, Erin Dahlgren wrote:
>
> > diff --git a/setup.c b/setup.c
> > index 1be5037..e1a9e17 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -831,16 +831,6 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
> > return NULL;
> > }
> >
> > -static const char *setup_nongit(const char *cwd, int *nongit_ok)
> > -{
> > - if (!nongit_ok)
> > - die(_("not a git repository (or any of the parent directories): %s"), DEFAULT_GIT_DIR_ENVIRONMENT);
> > - if (chdir(cwd))
> > - die_errno(_("cannot come back to cwd"));
> > - *nongit_ok = 1;
> > - return NULL;
> > -}
> > -
> > static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_len)
> > {
> > struct stat buf;
> > @@ -1097,18 +1087,20 @@ const char *setup_git_directory_gently(int *nongit_ok)
> > prefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);
> > break;
> > case GIT_DIR_HIT_CEILING:
> > - prefix = setup_nongit(cwd.buf, nongit_ok);
> > - break;
> > + if (!nongit_ok)
> > + die(_("not a git repository (or any of the parent directories): %s"),
> > + DEFAULT_GIT_DIR_ENVIRONMENT);
>
> I am terribly sorry to bother you about formatting issues (in my mind, it
> is quite an annoying thing that we still have no fully automatic way to
> format Git's source code according to Git's preferred coding style, but
> there you go...): this `DEFAULT_GIT_DIR_ENVIRONMENT` should be aligned
> with the first parameter of `die()`, i.e.
>
> + if (!nongit_ok)
> + die(_("not a git repository (or any of the parent directories): %s"),
> + DEFAULT_GIT_DIR_ENVIRONMENT);
>
> > + *nongit_ok = 1;
> > + strbuf_release(&dir);
> > + return NULL;
> > case GIT_DIR_HIT_MOUNT_POINT:
> > - if (nongit_ok) {
> > - *nongit_ok = 1;
> > - strbuf_release(&cwd);
> > - strbuf_release(&dir);
> > - return NULL;
> > - }
> > - die(_("not a git repository (or any parent up to mount point %s)\n"
> > - "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> > - dir.buf);
> > + if (!nongit_ok)
> > + die(_("not a git repository (or any parent up to mount point %s)\n"
> > + "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> > + dir.buf);
>
> Likewise, `dir.buf` should be aligned with the `_` two lines above.
Hi Johannes,
No problem, I'll make those changes.
I'd be interested to hear the arguments against a "fully automatic way
to format Git's source code according to Git's preferred coding
style". If there aren't any then I'd be willing to take a crack at it.
Should we start a separate "discussion" thread?
>
> Otherwise I think this patch is good to go!
>
> Thank you,
> Johannes
>
> > + *nongit_ok = 1;
> > + strbuf_release(&dir);
> > + return NULL;
> > default:
> > BUG("unhandled setup_git_directory_1() result");
> > }
> > --
> > 2.7.4
> >
> >
^ 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