* [PATCH] git-p4: do not pass '-r 0' to p4 commands
From: Igor Kushnir @ 2016-12-29 9:05 UTC (permalink / raw)
To: git; +Cc: Lars Schneider, Luke Diamand, Ori Rawlings, Igor Kushnir
git-p4 crashes when used with a very old p4 client version
that does not support the '-r <number>' option in its commands.
Allow making git-p4 work with old p4 clients by setting git-p4.retries to 0.
Alternatively git-p4.retries could be made opt-in.
But since only very old, barely maintained p4 versions don't support
the '-r' option, the setting-retries-to-0 workaround would do.
The "-r retries" option is present in Perforce 2012.2 Command Reference,
but absent from Perforce 2012.1 Command Reference.
Signed-off-by: Igor Kushnir <igorkuo@gmail.com>
---
Documentation/git-p4.txt | 1 +
git-p4.py | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index bae862ddc..f4f1be5be 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -479,6 +479,7 @@ git-p4.client::
git-p4.retries::
Specifies the number of times to retry a p4 command (notably,
'p4 sync') if the network times out. The default value is 3.
+ '-r 0' is not passed to p4 commands if this option is set to 0.
Clone and sync variables
~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/git-p4.py b/git-p4.py
index 22e3f57e7..e5a9e1cce 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -83,7 +83,9 @@ def p4_build_cmd(cmd):
if retries is None:
# Perform 3 retries by default
retries = 3
- real_cmd += ["-r", str(retries)]
+ if retries != 0:
+ # Provide a way to not pass this option by setting git-p4.retries to 0
+ real_cmd += ["-r", str(retries)]
if isinstance(cmd,basestring):
real_cmd = ' '.join(real_cmd) + ' ' + cmd
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] git-p4: do not pass '-r 0' to p4 commands
From: Luke Diamand @ 2016-12-29 9:20 UTC (permalink / raw)
To: Igor Kushnir; +Cc: Git Users, Lars Schneider, Ori Rawlings
In-Reply-To: <20161229090533.4717-1-igorkuo@gmail.com>
On 29 December 2016 at 09:05, Igor Kushnir <igorkuo@gmail.com> wrote:
> git-p4 crashes when used with a very old p4 client version
> that does not support the '-r <number>' option in its commands.
>
> Allow making git-p4 work with old p4 clients by setting git-p4.retries to 0.
>
> Alternatively git-p4.retries could be made opt-in.
> But since only very old, barely maintained p4 versions don't support
> the '-r' option, the setting-retries-to-0 workaround would do.
>
> The "-r retries" option is present in Perforce 2012.2 Command Reference,
> but absent from Perforce 2012.1 Command Reference.
That looks like a good fix, thanks.
I feel sad for anyone still using 2012.1.
Luke
>
> Signed-off-by: Igor Kushnir <igorkuo@gmail.com>
> ---
> Documentation/git-p4.txt | 1 +
> git-p4.py | 4 +++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index bae862ddc..f4f1be5be 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -479,6 +479,7 @@ git-p4.client::
> git-p4.retries::
> Specifies the number of times to retry a p4 command (notably,
> 'p4 sync') if the network times out. The default value is 3.
> + '-r 0' is not passed to p4 commands if this option is set to 0.
>
> Clone and sync variables
> ~~~~~~~~~~~~~~~~~~~~~~~~
> diff --git a/git-p4.py b/git-p4.py
> index 22e3f57e7..e5a9e1cce 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -83,7 +83,9 @@ def p4_build_cmd(cmd):
> if retries is None:
> # Perform 3 retries by default
> retries = 3
> - real_cmd += ["-r", str(retries)]
> + if retries != 0:
> + # Provide a way to not pass this option by setting git-p4.retries to 0
> + real_cmd += ["-r", str(retries)]
>
> if isinstance(cmd,basestring):
> real_cmd = ' '.join(real_cmd) + ' ' + cmd
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH] git-p4: do not pass '-r 0' to p4 commands
From: Lars Schneider @ 2016-12-29 10:02 UTC (permalink / raw)
To: Igor Kushnir; +Cc: git, Luke Diamand, Ori Rawlings
In-Reply-To: <20161229090533.4717-1-igorkuo@gmail.com>
> On 29 Dec 2016, at 10:05, Igor Kushnir <igorkuo@gmail.com> wrote:
>
> git-p4 crashes when used with a very old p4 client version
> that does not support the '-r <number>' option in its commands.
>
> Allow making git-p4 work with old p4 clients by setting git-p4.retries to 0.
>
> Alternatively git-p4.retries could be made opt-in.
> But since only very old, barely maintained p4 versions don't support
> the '-r' option, the setting-retries-to-0 workaround would do.
>
> The "-r retries" option is present in Perforce 2012.2 Command Reference,
> but absent from Perforce 2012.1 Command Reference.
Thanks for this workaround!
> Signed-off-by: Igor Kushnir <igorkuo@gmail.com>
> ---
> Documentation/git-p4.txt | 1 +
> git-p4.py | 4 +++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index bae862ddc..f4f1be5be 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -479,6 +479,7 @@ git-p4.client::
> git-p4.retries::
> Specifies the number of times to retry a p4 command (notably,
> 'p4 sync') if the network times out. The default value is 3.
> + '-r 0' is not passed to p4 commands if this option is set to 0.
At this point in the docs we have never talked about the "-r" flag and I
would argue it is an "implementation detail". Therefore, I would prefer
something like: "Set the value to 0 if want to disable retries or if
your p4 version does not support retries (pre 2012.2)."
>
> Clone and sync variables
> ~~~~~~~~~~~~~~~~~~~~~~~~
> diff --git a/git-p4.py b/git-p4.py
> index 22e3f57e7..e5a9e1cce 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -83,7 +83,9 @@ def p4_build_cmd(cmd):
> if retries is None:
> # Perform 3 retries by default
> retries = 3
> - real_cmd += ["-r", str(retries)]
> + if retries != 0:
How about "retries > 0"?
> + # Provide a way to not pass this option by setting git-p4.retries to 0
> + real_cmd += ["-r", str(retries)]
>
> if isinstance(cmd,basestring):
> real_cmd = ' '.join(real_cmd) + ' ' + cmd
> --
> 2.11.0
>
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2016, #08; Tue, 27)
From: Duy Nguyen @ 2016-12-29 10:06 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <20161228181808.GC33595@google.com>
On Thu, Dec 29, 2016 at 1:18 AM, Brandon Williams <bmwill@google.com> wrote:
> On 12/27, Junio C Hamano wrote:
>> * bw/pathspec-cleanup (2016-12-14) 16 commits
>> - pathspec: rename prefix_pathspec to init_pathspec_item
>> - pathspec: small readability changes
>> - pathspec: create strip submodule slash helpers
>> - pathspec: create parse_element_magic helper
>> - pathspec: create parse_long_magic function
>> - pathspec: create parse_short_magic function
>> - pathspec: factor global magic into its own function
>> - pathspec: simpler logic to prefix original pathspec elements
>> - pathspec: always show mnemonic and name in unsupported_magic
>> - pathspec: remove unused variable from unsupported_magic
>> - pathspec: copy and free owned memory
>> - pathspec: remove the deprecated get_pathspec function
>> - ls-tree: convert show_recursive to use the pathspec struct interface
>> - dir: convert fill_directory to use the pathspec struct interface
>> - dir: remove struct path_simplify
>> - mv: remove use of deprecated 'get_pathspec()'
>>
>> Code clean-up in the pathspec API.
>>
>> Waiting for the (hopefully) final round of review before 'next'.
>
> What more needs to be reviewed for this series?
I wanted to have a look at it but I successfully managed to put if off
so far. Will do soonish.
--
Duy
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2016, #08; Tue, 27)
From: Lars Schneider @ 2016-12-29 10:10 UTC (permalink / raw)
To: Junio C Hamano, bmwill; +Cc: git
In-Reply-To: <xmqqy3z155o1.fsf@gitster.mtv.corp.google.com>
> On 28 Dec 2016, at 00:11, Junio C Hamano <gitster@pobox.com> wrote:
>
>
> * bw/realpath-wo-chdir (2016-12-22) 5 commits
> (merged to 'next' on 2016-12-22 at fea8fa870f)
> + real_path: canonicalize directory separators in root parts
> + real_path: have callers use real_pathdup and strbuf_realpath
> + real_path: create real_pathdup
> + real_path: convert real_path_internal to strbuf_realpath
> + real_path: resolve symlinks by hand
> (this branch is used by bw/grep-recurse-submodules.)
>
> The implementation of "real_path()" was to go there with chdir(2)
> and call getcwd(3), but this obviously wouldn't be usable in a
> threaded environment. Rewrite it to manually resolve relative
> paths including symbolic links in path components.
"real_path: resolve symlinks by hand" (05b458c) introduces
"MAXSYMLINKS" which is already defined on macOS in
/usr/include/sys/param.h:197:9:
* .., MAXSYMLINKS defines the
* maximum number of symbolic links that may be expanded in a path name.
* It should be set high enough to allow all legitimate uses, but halt
* infinite loops reasonably quickly.
*/
Log with JS: https://travis-ci.org/git/git/jobs/187092215
Log without JS: https://s3.amazonaws.com/archive.travis-ci.org/jobs/187092215/log.txt
- Lars
^ permalink raw reply
* [PATCH v2] git-p4: do not pass '-r 0' to p4 commands
From: Igor Kushnir @ 2016-12-29 10:22 UTC (permalink / raw)
To: git; +Cc: Lars Schneider, Luke Diamand, Ori Rawlings, Igor Kushnir
git-p4 crashes when used with a very old p4 client version
that does not support the '-r <number>' option in its commands.
Allow making git-p4 work with old p4 clients by setting git-p4.retries to 0.
Alternatively git-p4.retries could be made opt-in.
But since only very old, barely maintained p4 versions don't support
the '-r' option, the setting-retries-to-0 workaround would do.
The "-r retries" option is present in Perforce 2012.2 Command Reference,
but absent from Perforce 2012.1 Command Reference.
Signed-off-by: Igor Kushnir <igorkuo@gmail.com>
---
Documentation/git-p4.txt | 2 ++
git-p4.py | 4 +++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index bae862ddc..7436c64a9 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -479,6 +479,8 @@ git-p4.client::
git-p4.retries::
Specifies the number of times to retry a p4 command (notably,
'p4 sync') if the network times out. The default value is 3.
+ Set the value to 0 to disable retries or if your p4 version
+ does not support retries (pre 2012.2).
Clone and sync variables
~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/git-p4.py b/git-p4.py
index 22e3f57e7..7bda915bd 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -83,7 +83,9 @@ def p4_build_cmd(cmd):
if retries is None:
# Perform 3 retries by default
retries = 3
- real_cmd += ["-r", str(retries)]
+ if retries > 0:
+ # Provide a way to not pass this option by setting git-p4.retries to 0
+ real_cmd += ["-r", str(retries)]
if isinstance(cmd,basestring):
real_cmd = ' '.join(real_cmd) + ' ' + cmd
--
2.11.0
^ permalink raw reply related
* Re: HowTo distribute a hook with the reposity.
From: Lars Schneider @ 2016-12-29 10:29 UTC (permalink / raw)
To: Jacob Keller; +Cc: Jeff King, John P. Hartmann, Git mailing list
In-Reply-To: <CA+P7+xoBpAV1hiZQ0WZHP-kbB9zhddMhtj_CZ1Wejdur3oPXhQ@mail.gmail.com>
> On 28 Dec 2016, at 19:53, Jacob Keller <jacob.keller@gmail.com> wrote:
>
> On Tue, Dec 27, 2016 at 10:08 PM, Jeff King <peff@peff.net> wrote:
>>
>> https://github.com/Autodesk/enterprise-config-for-git
>>
>> (with the disclaimer that I've never used it myself, so I have no
>> idea how good it is).
>>
>> I think you probably know all that, Jake, but I am laying it out for the
>> benefit of the OP and the list. :)
>>
>
> Heh, well I didn't know about this last part so it's still useful to
> me! I knew of the larger description for why not but wasn't exactly
> clear how to word it so I am glad you filled that in. Thanks!
Author of the "enterprise-config-for-git" here. The version in the public
Git repo is a stripped down version of the one we use at my day job for
our engineering workforce. I tried to extract the "generally useful bits".
Unfortunately it cannot be used "as-is". Don't hesitate to ping me if you
want to learn more about it and/or if you have an idea how to make it
better suited for open source collaboration.
Cheers,
Lars
^ permalink raw reply
* Re: [PATCH] unpack-trees: move checkout state into check_updates
From: René Scharfe @ 2016-12-29 13:53 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git
In-Reply-To: <20161228232616.21109-1-sbeller@google.com>
Am 29.12.2016 um 00:26 schrieb Stefan Beller:
> The checkout state was introduced via 16da134b1f9
> (read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to
> refactor the checkout state was done in b56aa5b268e (unpack-trees: pass
> checkout state explicitly to check_updates(), 2016-09-13), but we can
> go even further.
>
> The `struct checkout state` is not used in unpack_trees apart from
> initializing it, so move it into the function that makes use of it,
> which is `check_updates`.
Thanks for catching this, the result looks nicer.
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> unpack-trees.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index ea799d37c5..78703af135 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -224,14 +224,18 @@ static void unlink_entry(const struct cache_entry *ce)
> schedule_dir_for_removal(ce->name, ce_namelen(ce));
> }
>
> -static int check_updates(struct unpack_trees_options *o,
> - const struct checkout *state)
> +static int check_updates(struct unpack_trees_options *o)
> {
> unsigned cnt = 0, total = 0;
> struct progress *progress = NULL;
> struct index_state *index = &o->result;
> int i;
> int errs = 0;
> + struct checkout state = CHECKOUT_INIT;
> + state.force = 1;
> + state.quiet = 1;
> + state.refresh_cache = 1;
> + state.istate = &o->result;
And here (as well as in two more places in this function) we could use
"index" instead of "&o->result". Not sure if it's worth a patch, though.
>
> if (o->update && o->verbose_update) {
> for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
> @@ -270,7 +274,7 @@ static int check_updates(struct unpack_trees_options *o,
> display_progress(progress, ++cnt);
> ce->ce_flags &= ~CE_UPDATE;
> if (o->update && !o->dry_run) {
> - errs |= checkout_entry(ce, state, NULL);
> + errs |= checkout_entry(ce, &state, NULL);
> }
> }
> }
> @@ -1100,14 +1104,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> int i, ret;
> static struct cache_entry *dfc;
> struct exclude_list el;
> - struct checkout state = CHECKOUT_INIT;
>
> if (len > MAX_UNPACK_TREES)
> die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
> - state.force = 1;
> - state.quiet = 1;
> - state.refresh_cache = 1;
> - state.istate = &o->result;
>
> memset(&el, 0, sizeof(el));
> if (!core_apply_sparse_checkout || !o->update)
> @@ -1244,7 +1243,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> }
>
> o->src_index = NULL;
> - ret = check_updates(o, &state) ? (-2) : 0;
> + ret = check_updates(o) ? (-2) : 0;
> if (o->dst_index) {
> if (!ret) {
> if (!o->result.cache_tree)
>
^ permalink raw reply
* Re: [PATCH] string-list: make compare function compatible with qsort(3)
From: René Scharfe @ 2016-12-29 13:53 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano, Johannes Schindelin
In-Reply-To: <20161221161220.x3qkcwmuangcdc2l@sigill.intra.peff.net>
Am 21.12.2016 um 17:12 schrieb Jeff King:
> On Wed, Dec 21, 2016 at 10:36:41AM +0100, René Scharfe wrote:
>
>> One shortcoming is that the comparison function is restricted to working
>> with the string members of items; util is inaccessible to it. Another
>> one is that the value of cmp is passed in a global variable to
>> cmp_items(), making string_list_sort() non-reentrant.
>
> I think this approach is OK for string_list, but it doesn't help the
> general case that wants qsort_s() to actually access global data. I
> don't know how common that is in our codebase, though.
>
> So I'm fine with it, but I think we might eventually need to revisit the
> qsort_s() thing anyway.
I have to admit I didn't even consider the possibility that the pattern
of accessing global variables in qsort(1) compare functions could have
spread.
And indeed, at least ref-filter.c::compare_refs() and
worktree.c::compare_worktree() so that as well. The latter uses
fspathcmp(), which is OK as ignore_case is only set once when reading
the config, but the first one looks, well, interesting. Perhaps a
single ref filter per program is enough?
Anyway, that's a good enough argument for me for adding that newfangled
C11 function after all..
Btw.: Found with
git grep 'QSORT.*;$' |
sed 's/.* /int /; s/);//' |
sort -u |
git grep -Ww -f-
and
git grep -A1 'QSORT.*,$'
>> Remove the intermediate layer, i.e. cmp_items(), make the comparison
>> functions compatible with qsort(3) and pass them pointers to full items.
>> This allows comparisons to also take the util member into account, and
>> avoids the need to pass the real comparison function to an intermediate
>> function, removing the need for a global function.
>
> I'm not sure if access to the util field is really of any value, after
> looking at it in:
>
> http://public-inbox.org/git/20161125171546.fa3zpapbjngjcl26@sigill.intra.peff.net/
>
> Though note that if we do take this patch, there are probably one or two
> spots that could switch from QSORT() to string_list_sort().
Yes, but as you noted in that thread there is not much point in doing
that; the only net win is that we can pass a list as a single pointer
instead of as base pointer and element count -- the special compare
function needs to be specified anyway (once), somehow.
René
^ permalink raw reply
* Re: [PATCH v2] am: add am.signoff add config variable
From: Eduardo Habkost @ 2016-12-29 15:37 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Stefan Beller, git@vger.kernel.org, Paul Tan
In-Reply-To: <CAFZEwPOPMrCXTc+SMhjGSnPKLmefcde4MgJsz7n5rBApACZOug@mail.gmail.com>
On Thu, Dec 29, 2016 at 01:29:33PM +0530, Pranit Bauva wrote:
> Hey Eduardo,
>
> On Thu, Dec 29, 2016 at 12:49 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> test_expect_success '--no-signoff overrides am.signoff' '
> >> rm -fr .git/rebase-apply &&
> >> git reset --hard first &&
> >> test_config am.signoff true &&
> >> git am --no-signoff <patch2 &&
> >> printf "%s\n" "$signoff" >expected &&
> >> git cat-file commit HEAD^ | grep "Signed-off-by:" >actual &&
> >> test_cmp expected actual &&
> >> git cat-file commit HEAD | grep "Signed-off-by:" >actual &&
> >> test_must_be_empty actual
> >> '
> >>
> >> The test fails because the second "grep" command returns a
> >> non-zero exit code. Any suggestions to avoid that problem in a
> >> more idiomatic way?
> >
> > I just found out that "test_must_fail grep ..." is a common
> > idiom, so what about:
>
> Is there any particular reason to use "grep" instead of "test_cmp"? To
> check for non-zero error code, you can always use "! test_cmp".
The test code is checking only the "Signed-off-by" lines, not the
whole commit message. "test_cmp" would require recovering the
entire contents of the original commit message, which would add
complexity to the test code.
--
Eduardo
^ permalink raw reply
* Re: [PATCH] am: add am.signoff add config variable
From: Eduardo Habkost @ 2016-12-29 15:45 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Paul Tan
In-Reply-To: <20161229084701.GA3643@starla>
On Thu, Dec 29, 2016 at 08:47:01AM +0000, Eric Wong wrote:
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > git-am has options to enable --message-id and --3way by default,
> > but no option to enable --signoff by default. Add a "am.signoff"
> > config option.
>
> I'm not sure this is a good idea. IANAL, but a sign-off
> has some sort of legal meaning for this project (DCO)
> and that would be better decided on a patch-by-patch basis
> rather than a blanket statement.
>
> I don't add my SoB to patches (either my own or received) until
> I'm comfortable with it; and I'd rather err on the side of
> forgetting and being prodded to resubmit rather than putting
> an SoB on the wrong patch.
This is an interesting point. I am not competent to argue about
it, so I will let the Git developers decide.
--
Eduardo
^ permalink raw reply
* Re: [PATCH v3] am: add am.signoff add config variable
From: Eduardo Habkost @ 2016-12-29 15:49 UTC (permalink / raw)
To: Andreas Schwab; +Cc: git, Paul Tan, Stefan Beller, Eric Wong
In-Reply-To: <m2wpejb1zn.fsf@linux-m68k.org>
On Thu, Dec 29, 2016 at 08:58:36AM +0100, Andreas Schwab wrote:
> On Dez 28 2016, Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > @@ -32,10 +32,12 @@ OPTIONS
> > If you supply directories, they will be treated as Maildirs.
> >
> > -s::
> > ---signoff::
> > +--[no-]-signoff::
>
> That's one dash too much.
Oops. I can fix it in v4, but I will first wait to see what
others think about the legal implications of setting it by
default (see point raised by Eric Wong on v1).
--
Eduardo
^ permalink raw reply
* Re: [PATCH] am: add am.signoff add config variable
From: Jacob Keller @ 2016-12-29 17:49 UTC (permalink / raw)
To: Eric Wong, Eduardo Habkost; +Cc: git, Paul Tan
In-Reply-To: <20161229084701.GA3643@starla>
On December 29, 2016 12:47:01 AM PST, Eric Wong <e@80x24.org> wrote:
>Eduardo Habkost <ehabkost@redhat.com> wrote:
>> git-am has options to enable --message-id and --3way by default,
>> but no option to enable --signoff by default. Add a "am.signoff"
>> config option.
>
>I'm not sure this is a good idea. IANAL, but a sign-off
>has some sort of legal meaning for this project (DCO)
>and that would be better decided on a patch-by-patch basis
>rather than a blanket statement.
>
>I don't add my SoB to patches (either my own or received) until
>I'm comfortable with it; and I'd rather err on the side of
>forgetting and being prodded to resubmit rather than putting
>an SoB on the wrong patch.
>
>
>(I'm barely online today, no rush needed in responding)
I don't know what is true for all projects, but I can't believe making it configurable would cause problems.... If you don't want it you don't have to enable it.
I suppose your argument is that we shouldn't ever make it automatically but... I know that many people who receive email patches, apply them, then forward those to another group add their sign off as a mark of "yes I certify that this is legally OK" so I think this would be useful.
Thanks
Jake
^ permalink raw reply
* [PATCH] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2016-12-29 19:29 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, peff, Stefan Beller
Every once in a while someone complains to the mailing list to have
run into this weird assertion[1].
The usual response from the mailing list is link to old discussions[2],
and acknowledging the problem stating it is known.
For now just improve the user visible error message.
[1] https://www.google.com/search?q=item-%3Enowildcard_len
[2] http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
https://www.spinics.net/lists/git/msg249473.html
Signed-off-by: Stefan Beller <sbeller@google.com>
---
* a more defensive check and message
* with tests!
pathspec.c | 19 +++++++++++++++++--
t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+), 2 deletions(-)
create mode 100755 t/t6134-pathspec-in-submodule.sh
diff --git a/pathspec.c b/pathspec.c
index 22ca74a126..b446d79615 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -313,8 +313,23 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
/* sanity checks, pathspec matchers assume these are sane */
- assert(item->nowildcard_len <= item->len &&
- item->prefix <= item->len);
+ if (item->nowildcard_len > item->len ||
+ item->prefix > item->len) {
+ /* Historically this always was a submodule issue */
+ for (i = 0; i < active_nr; i++) {
+ struct cache_entry *ce = active_cache[i];
+ int ce_len = ce_namelen(ce);
+ int len = ce_len < item->len ? ce_len : item->len;
+ if (!S_ISGITLINK(ce->ce_mode))
+ continue;
+ if (!memcmp(ce->name, item->match, len))
+ die (_("Pathspec '%s' is in submodule '%.*s'"),
+ item->original, ce_len, ce->name);
+ }
+ /* The error is a new unknown bug */
+ die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
+ }
+
return magic;
}
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
new file mode 100755
index 0000000000..e62dbb7327
--- /dev/null
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='test case exclude pathspec'
+
+. ./test-lib.sh
+
+test_expect_success 'setup a submodule' '
+ test_commit 1 &&
+ git submodule add ./ sub &&
+ git commit -a -m "add submodule" &&
+ git submodule deinit --all
+'
+
+cat <<EOF >expect
+fatal: Pathspec 'sub/a' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule' '
+ echo a >sub/a &&
+ test_must_fail git add sub/a 2>actual &&
+ test_cmp expect actual
+'
+
+cat <<EOF >expect
+fatal: Pathspec '.' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule from within submodule' '
+ test_must_fail git -C sub add . 2>actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.11.0.259.ga95e92af08.dirty
^ permalink raw reply related
* [PATCHv2] unpack-trees: move checkout state into check_updates
From: Stefan Beller @ 2016-12-29 19:43 UTC (permalink / raw)
To: gitster; +Cc: git, l.s.r, Stefan Beller
The checkout state was introduced via 16da134b1f9
(read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to
refactor the checkout state was done in b56aa5b268e (unpack-trees: pass
checkout state explicitly to check_updates(), 2016-09-13), but we can
go even further.
The `struct checkout state` is not used in unpack_trees apart from
initializing it, so move it into the function that makes use of it,
which is `check_updates`.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Rene wrote:
> we could use "index" instead of "&o->result". Not sure if it's worth a patch, though.
done in this iteration of the patch.
I also reordered the variables at the beginning of the function for readability.
Thanks,
Stefan
unpack-trees.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 7a6df99d10..eb44f50e65 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -218,14 +218,18 @@ static void unlink_entry(const struct cache_entry *ce)
schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
-static int check_updates(struct unpack_trees_options *o,
- const struct checkout *state)
+static int check_updates(struct unpack_trees_options *o)
{
unsigned cnt = 0, total = 0;
struct progress *progress = NULL;
struct index_state *index = &o->result;
- int i;
- int errs = 0;
+ struct checkout state = CHECKOUT_INIT;
+ int i, errs = 0;
+
+ state.force = 1;
+ state.quiet = 1;
+ state.refresh_cache = 1;
+ state.istate = index;
if (o->update && o->verbose_update) {
for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
@@ -240,7 +244,7 @@ static int check_updates(struct unpack_trees_options *o,
}
if (o->update)
- git_attr_set_direction(GIT_ATTR_CHECKOUT, &o->result);
+ git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
for (i = 0; i < index->cache_nr; i++) {
const struct cache_entry *ce = index->cache[i];
@@ -251,7 +255,7 @@ static int check_updates(struct unpack_trees_options *o,
continue;
}
}
- remove_marked_cache_entries(&o->result);
+ remove_marked_cache_entries(index);
remove_scheduled_dirs();
for (i = 0; i < index->cache_nr; i++) {
@@ -264,7 +268,7 @@ static int check_updates(struct unpack_trees_options *o,
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
if (o->update && !o->dry_run) {
- errs |= checkout_entry(ce, state, NULL);
+ errs |= checkout_entry(ce, &state, NULL);
}
}
}
@@ -1094,14 +1098,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
int i, ret;
static struct cache_entry *dfc;
struct exclude_list el;
- struct checkout state = CHECKOUT_INIT;
if (len > MAX_UNPACK_TREES)
die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
- state.force = 1;
- state.quiet = 1;
- state.refresh_cache = 1;
- state.istate = &o->result;
memset(&el, 0, sizeof(el));
if (!core_apply_sparse_checkout || !o->update)
@@ -1238,7 +1237,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
}
o->src_index = NULL;
- ret = check_updates(o, &state) ? (-2) : 0;
+ ret = check_updates(o) ? (-2) : 0;
if (o->dst_index) {
if (!ret) {
if (!o->result.cache_tree)
--
2.11.0.259.ga95e92af08.dirty
^ permalink raw reply related
* Re: [PATCH] am: add am.signoff add config variable
From: Junio C Hamano @ 2016-12-29 21:42 UTC (permalink / raw)
To: Eric Wong; +Cc: Eduardo Habkost, git, Paul Tan
In-Reply-To: <20161229084701.GA3643@starla>
Eric Wong <e@80x24.org> writes:
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>> git-am has options to enable --message-id and --3way by default,
>> but no option to enable --signoff by default. Add a "am.signoff"
>> config option.
>
> I'm not sure this is a good idea. IANAL, but a sign-off
> has some sort of legal meaning for this project (DCO)
> and that would be better decided on a patch-by-patch basis
> rather than a blanket statement.
IANAL either, but we have been striving to keep output of
$ git grep '\.signoff' Documentation
empty to keep Sign-off meaningful.
Adding more publicized ways to add SoB without thinking will make it
harder to argue against one who tells the court "that log message
ends with a SoB by person X but it is very plausible that it was
done by inertia without person X really intending to certify what
DCO says, and the SoB is meaningless".
> I don't add my SoB to patches (either my own or received) until
> I'm comfortable with it; and I'd rather err on the side of
> forgetting and being prodded to resubmit rather than putting
> an SoB on the wrong patch.
^ permalink raw reply
* Re: [PATCH] am: add am.signoff add config variable
From: Eduardo Habkost @ 2016-12-29 22:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Wong, git, Paul Tan
In-Reply-To: <xmqqtw9m5s5m.fsf@gitster.mtv.corp.google.com>
On Thu, Dec 29, 2016 at 01:42:13PM -0800, Junio C Hamano wrote:
> Eric Wong <e@80x24.org> writes:
>
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> git-am has options to enable --message-id and --3way by default,
> >> but no option to enable --signoff by default. Add a "am.signoff"
> >> config option.
> >
> > I'm not sure this is a good idea. IANAL, but a sign-off
> > has some sort of legal meaning for this project (DCO)
> > and that would be better decided on a patch-by-patch basis
> > rather than a blanket statement.
>
> IANAL either, but we have been striving to keep output of
>
> $ git grep '\.signoff' Documentation
>
> empty to keep Sign-off meaningful.
>
> Adding more publicized ways to add SoB without thinking will make it
> harder to argue against one who tells the court "that log message
> ends with a SoB by person X but it is very plausible that it was
> done by inertia without person X really intending to certify what
> DCO says, and the SoB is meaningless".
This sounds completely reasonable to me. I now see that the
config option was already proposed in 2011 and the same arguments
were discussed. Sorry for the noise.
>
> > I don't add my SoB to patches (either my own or received) until
> > I'm comfortable with it; and I'd rather err on the side of
> > forgetting and being prodded to resubmit rather than putting
> > an SoB on the wrong patch.
>
--
Eduardo
^ permalink raw reply
* Re: [PATCH] am: add am.signoff add config variable
From: Stefan Beller @ 2016-12-29 22:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Wong, Eduardo Habkost, git@vger.kernel.org, Paul Tan
In-Reply-To: <xmqqtw9m5s5m.fsf@gitster.mtv.corp.google.com>
On Thu, Dec 29, 2016 at 1:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
>
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>> git-am has options to enable --message-id and --3way by default,
>>> but no option to enable --signoff by default. Add a "am.signoff"
>>> config option.
>>
>> I'm not sure this is a good idea. IANAL, but a sign-off
>> has some sort of legal meaning for this project (DCO)
>> and that would be better decided on a patch-by-patch basis
>> rather than a blanket statement.
>
> IANAL either, but we have been striving to keep output of
>
> $ git grep '\.signoff' Documentation
Try again with -i ;)
and you'll find format.signOff
>
> empty to keep Sign-off meaningful.
>
> Adding more publicized ways to add SoB without thinking will make it
> harder to argue against one who tells the court "that log message
> ends with a SoB by person X but it is very plausible that it was
> done by inertia without person X really intending to certify what
> DCO says, and the SoB is meaningless".
I think we should be symmetrical, am is the opposite of
format-patch in the Git <-> email conversion.
If I were to follow your arguments here, we should revert
1d1876e930 (Add configuration variable for sign-off to format-patch)
On the other hand, I would argue that thinking and typing things is
orthogonal (the more you type, doesn't imply that you think harder
or even at all).
--
However I think there is a use case for such an option
(in the short term?) as e.g. you as the Git maintainer being employed
has the legal rights to sign off on pretty much any patch in Git.
For other projects this may be different.
Which is why a per-repository configurable thing is useful to
setup a default for your environment.
IIUC long term we rather want to have easily configurable trailers
for am/format-patch/commit, such that you could configure to
remove any "ChangeId:" footers before sending out, or adding
a "Tested-by" footer by a CI system or such?
>
>> I don't add my SoB to patches (either my own or received) until
>> I'm comfortable with it;
comfortable is orthogonal to legal, specifically on the receiving side.
I can understand using format-patch to send out unsigned patches
(e.g. for heavy WIP things, "please to not apply literally")
>> and I'd rather err on the side of
>> forgetting and being prodded to resubmit rather than putting
>> an SoB on the wrong patch.
>
^ permalink raw reply
* Re: [PATCH] am: add am.signoff add config variable
From: Junio C Hamano @ 2016-12-29 22:43 UTC (permalink / raw)
To: Stefan Beller; +Cc: Eric Wong, Eduardo Habkost, git@vger.kernel.org, Paul Tan
In-Reply-To: <CAGZ79kaAbCTsY_SddVMKMsLV0xyXNBFvxQ=J-20Cwdz31v4OwA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> IANAL either, but we have been striving to keep output of
>>
>> $ git grep '\.signoff' Documentation
>
>>
>> empty to keep Sign-off meaningful.
>
> Try again with -i ;)
> and you'll find format.signOff
Mistakes happen. Finding an old mistake is not an excuse for you to
make the same one again. It is an opportunity to come up with a way
to correct it without hurting existing users by designing a smooth
transition path.
^ permalink raw reply
* Re: [PATCH] am: add am.signoff add config variable
From: Stefan Beller @ 2016-12-29 23:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Wong, Eduardo Habkost, git@vger.kernel.org, Paul Tan
In-Reply-To: <xmqqpoka5pb0.fsf@gitster.mtv.corp.google.com>
On Thu, Dec 29, 2016 at 2:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> IANAL either, but we have been striving to keep output of
>>>
>>> $ git grep '\.signoff' Documentation
>>
>>>
>>> empty to keep Sign-off meaningful.
>>
>> Try again with -i ;)
>> and you'll find format.signOff
>
> Mistakes happen. Finding an old mistake is not an excuse for you to
> make the same one again.
> It is an opportunity to come up with a way
> to correct it without hurting existing users by designing a smooth
> transition path.
>
knee jerk reaction:
1) Document why format.signoff is bad (even after a long
office discussion I am not fully convinced it is bad. That
may be because I am biased as I find format.signoff *very*
useful. The cumbersome contribution process as laid out
by SubmittingPatches just got easier for me as I have one
step less to worry about. I haven't made a mistake so far
sending out crap where I'll throw a temper tantrum if you
apply it.
So I would expect a maintainer of a project that uses
email based workflow to write that documentation giving
reasons. That person is currently consuming the automatically
signed off patches, so I'd want to know their line of thinking.
2) If the config option is set, but no explicit sign off is given,
put a different footer, e.g.
git config format.signoff true &&
git format-patch HEAD^
may produce Auto-Signed-Off-By: ...
whereas
git -c format.signoff format-patch
behaves the same as
git format-patch --signoff
that gives the Signed-Off-By as we know it.
It is up to the upstream project to accept these new sign offs.
3) (later) warn about the option if it is set, giving the text from 1)
4) (a long time later) remove the option.
--
For 2) I am not sure what we want there, because this
has to happen in collaboration with all the upstream projects that
use sign offs.
We could be subtle, i.e. just use all lowercase / all uppercase
letters for this differentiation. Then automated tools that check for signoff
are easily adjusted. e.g. The eclipse foundation disallows pushing for
review if any patch is missing a signoff; Gerrit can check for that.
Gerrit is not case sensitive when checking for a footer.
I am not sure if there are any other tools out there that automatically
check for that, but I would assume they are also case insensitive in
such a case, as it is unclear to me how to properly capitalize the sign off.
This is an easy way forward for upstream projects., though confusing in
court later on.
--
We could also be non-subtle, very explicit, and each tool that
can add sign offs currently, needs to be explicit about itself:
Configured-Formatpatch-Signed-Off: (for git format.signoff with config)
# and others:
Explicit-Formatpatch-Signed-Off:
Git-Gui-Button-Clicked-Signed-Off:
Git-Gui-Button-Shortcut-Signed-Off:
Once we have that we could add much more of these:
Configured-Commit-Signed-Off:
etc.
You can continue to sign off via just typing it, or by
I-typed-it-signed-Off,
--
One of the problems highlighted to me was that you could have accidentally
configured format.signoff globally, but you're only allowed/desire to sign off
in a particular repository, such that
Repolocal-Configured-Formatpatch-Signed-Off:
Global-Configured-Formatpatch-Signed-Off:
may be worth discussing.
--
^ permalink raw reply
* Bug: Assertion failed: function prefix_pathspec, file pathspec.c, line 317.
From: Rafal W @ 2016-12-29 23:37 UTC (permalink / raw)
To: git@vger.kernel.org
The following error happens when I'm running "git add ." in the submodule dir:
$ GIT_TRACE=1 git add .
23:25:18.313575 git.c:350 trace: built-in: git 'add' '.'
Assertion failed: (item->nowildcard_len <= item->len && item->prefix
<= item->len), function prefix_pathspec, file pathspec.c, line 317.
Abort trap: 6
Then git crashes, so I've to remove index.lock manually
(.git/modules/Foo/index.lock).
This is what I did before it start doing this:
1. I've renamed GitHub repository to different name.
2. Created new repository in place of the old one (which consist other
submodules).
3. In previously cloned submodule dir (which was pointing to previous
repo before rename) I did: git fetch origin
4. Then I did: git reset origin/master --hard
5. 'git status' looks ok
6. Moved some of the files into submodule dir of the current submodule
and entered that dir.
7. From now on, the git add crashes with trap 6: git add .
Probably there are some easier steps to reproduce the issue, but this
is what I did.
OS: macOS Sierra
git version 2.8.4
(lldb) bt
* thread #1: tid = 0x2338e, 0x00007fffbe6a1dda
libsystem_kernel.dylib`__pthread_kill + 10, queue =
'com.apple.main-thread', stop reason = signal SIGABRT
* frame #0: 0x00007fffbe6a1dda libsystem_kernel.dylib`__pthread_kill + 10
frame #1: 0x00007fffbe78c797 libsystem_pthread.dylib`pthread_kill + 90
frame #2: 0x00007fffbe607440 libsystem_c.dylib`abort + 129
frame #3: 0x00007fffbe5ce8b3 libsystem_c.dylib`__assert_rtn + 320
frame #4: 0x00000001000d83f9 git`parse_pathspec + 2917
frame #5: 0x0000000100002ded git`cmd_add + 991
frame #6: 0x0000000100001e64 git`handle_builtin + 357
frame #7: 0x0000000100001877 git`main + 288
Other people could reproduce the same thing, also on Mac, see:
http://stackoverflow.com/q/23205961
Kind regards,
kenorb
^ permalink raw reply
* Re: Bug: Assertion failed: function prefix_pathspec, file pathspec.c, line 317.
From: Stefan Beller @ 2016-12-29 23:45 UTC (permalink / raw)
To: Rafal W; +Cc: git@vger.kernel.org
In-Reply-To: <CANmdXCEGTxgj8BuQZkaBQaktOBrGmLETcQJg9xfV27Kn-0aLGA@mail.gmail.com>
On Thu, Dec 29, 2016 at 3:37 PM, Rafal W <kenorb@gmail.com> wrote:
> The following error happens when I'm running "git add ." in the submodule dir:
>
Thanks for reporting!
Please see the patch that I sent out earlier today:
https://public-inbox.org/git/20161229192908.32633-1-sbeller@google.com/
I wonder if the error message is sufficient, i.e. would you liked to have
more help reported by git?
^ permalink raw reply
* [PATCH v2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR
From: Jonathan Nieder @ 2016-12-30 0:11 UTC (permalink / raw)
To: Jeff King; +Cc: git, Duy Nguyen, Junio C Hamano, Stefan Beller
In-Reply-To: <20161122024102.otlnl6jcrb3pejux@sigill.intra.peff.net>
To push from or fetch to the current repository, remote helpers need
to know what repository that is. Accordingly, Git sets the GIT_DIR
environment variable to the path to the current repository when
invoking remote helpers.
There is a special case it does not handle: "git ls-remote" and "git
archive --remote" can be run to inspect a remote repository without
being run from any local repository. GIT_DIR is not useful in this
scenario:
- if we are not in a repository, we don't need to set GIT_DIR to
override an existing GIT_DIR value from the environment. If GIT_DIR
is present then we would be in a repository if it were valid and
would have called die() if it weren't.
- not setting GIT_DIR may cause a helper to do the usual discovery
walk to find the repository. But we know we're not in one, or we
would have found it ourselves. So in the worst case it may expend
a little extra effort to try to find a repository and fail (for
example, remote-curl would do this to try to find repository-level
configuration).
So leave GIT_DIR unset in this case. This makes GIT_DIR easier to
understand for remote helper authors and makes transport code less of
a special case for repository discovery.
Noticed using b1ef400e (setup_git_env: avoid blind fall-back to
".git", 2016-10-20) from 'next':
$ cd /tmp
$ git ls-remote https://kernel.googlesource.com/pub/scm/git/git
fatal: BUG: setup_git_env called without repository
While at it, add some tests for other variables in the environment of
commands run by git-remote-ext.
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jeff King wrote:
> So yeah, I think this is the extent of the change needed.
Thanks. Here's the patch again, now with commit messages and a test.
Thanks for the analysis and sorry for the slow turnaround.
Sincerely,
Jonathan
t/t5550-http-fetch-dumb.sh | 15 ++++++++
t/t5551-http-fetch-smart.sh | 15 ++++++++
t/t5570-git-daemon.sh | 15 ++++++++
t/t5802-connect-helper.sh | 84 ++++++++++++++++++++++++++++++++++++++++++++-
transport-helper.c | 5 +--
5 files changed, 131 insertions(+), 3 deletions(-)
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index aeb3a63f7c..a86fca3e6c 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -34,6 +34,21 @@ test_expect_success 'clone http repository' '
test_cmp file clone/file
'
+test_expect_success 'list refs from outside any repository' '
+ cat >expect <<-EOF &&
+ $(git rev-parse master) HEAD
+ $(git rev-parse master) refs/heads/master
+ EOF
+ mkdir lsremote-root &&
+ (
+ GIT_CEILING_DIRECTORIES=$(pwd) &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd lsremote-root &&
+ git ls-remote "$HTTPD_URL/dumb/repo.git" >../actual
+ ) &&
+ test_cmp expect actual
+'
+
test_expect_success 'create password-protected repository' '
mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/" &&
cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 6e5b9e42fb..7ba894f0f4 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -163,6 +163,21 @@ test_expect_success 'redirects send auth to new location' '
expect_askpass both user@host auth/smart/repo.git
'
+test_expect_success 'list refs from outside any repository' '
+ cat >expect <<-EOF &&
+ $(git rev-parse master) HEAD
+ $(git rev-parse master) refs/heads/master
+ EOF
+ mkdir lsremote-root &&
+ (
+ GIT_CEILING_DIRECTORIES=$(pwd) &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd lsremote-root &&
+ git ls-remote "$HTTPD_URL/smart/repo.git" >../actual
+ ) &&
+ test_cmp expect actual
+'
+
test_expect_success 'disable dumb http on server' '
git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
config http.getanyfile false
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 225a022e8a..4573d98e6c 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -35,6 +35,21 @@ test_expect_success 'clone git repository' '
test_cmp file clone/file
'
+test_expect_success 'list refs from outside any repository' '
+ cat >expect <<-EOF &&
+ $(git rev-parse master) HEAD
+ $(git rev-parse master) refs/heads/master
+ EOF
+ mkdir lsremote-root &&
+ (
+ GIT_CEILING_DIRECTORIES=$(pwd) &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd lsremote-root &&
+ git ls-remote "$GIT_DAEMON_URL/repo.git" >../actual
+ ) &&
+ test_cmp expect actual
+'
+
test_expect_success 'fetch changes via git protocol' '
echo content >>file &&
git commit -a -m two &&
diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
index c6c2661878..7232032cd2 100755
--- a/t/t5802-connect-helper.sh
+++ b/t/t5802-connect-helper.sh
@@ -3,6 +3,10 @@
test_description='ext::cmd remote "connect" helper'
. ./test-lib.sh
+escape_spaces () {
+ echo "$*" | sed -e "s/ /% /g"
+}
+
test_expect_success setup '
git config --global protocol.ext.allow user &&
test_tick &&
@@ -19,7 +23,7 @@ test_expect_success setup '
'
test_expect_success clone '
- cmd=$(echo "echo >&2 ext::sh invoked && %S .." | sed -e "s/ /% /g") &&
+ cmd=$(escape_spaces "echo >&2 ext::sh invoked && %S ..") &&
git clone "ext::sh -c %S% ." dst &&
git for-each-ref refs/heads/ refs/tags/ >expect &&
(
@@ -70,6 +74,84 @@ test_expect_success 'update backfilled tag without primary transfer' '
test_cmp expect actual
'
+test_expect_success 'GIT_EXT_SERVICE for clone, ls-remote, push, archive' '
+ rm -rf dst &&
+ >actual &&
+ cat >expect <<-\EOF &&
+ git-upload-pack
+ git-upload-pack
+ git-receive-pack
+ git-upload-archive
+ EOF
+ git archive HEAD >expect.tar &&
+ cmd=$(escape_spaces "echo >>actual \$GIT_EXT_SERVICE && %S .") &&
+
+ git clone "ext::sh -c $cmd" dst &&
+ git ls-remote "ext::sh -c $cmd" &&
+ test_when_finished "git update-ref -d refs/heads/newbranch" &&
+ git push "ext::sh -c $cmd" master:newbranch &&
+ git archive --remote="ext::sh -c $cmd" HEAD >actual.tar &&
+
+ test_cmp expect actual &&
+ test_cmp expect.tar actual.tar
+'
+
+test_expect_success 'GIT_EXT_SERVICE_NOPREFIX for clone, ls-remote, push, archive' '
+ rm -rf dst &&
+ >actual &&
+ cat >expect <<-\EOF &&
+ upload-pack
+ upload-pack
+ receive-pack
+ upload-archive
+ EOF
+ git archive HEAD >expect.tar &&
+ cmd=$(escape_spaces "echo >>actual \$GIT_EXT_SERVICE_NOPREFIX && %S .") &&
+
+ git clone "ext::sh -c $cmd" dst &&
+ git ls-remote "ext::sh -c $cmd" &&
+ test_when_finished "git update-ref -d refs/heads/newbranch" &&
+ git push "ext::sh -c $cmd" master:newbranch &&
+ git archive --remote="ext::sh -c $cmd" HEAD >actual.tar &&
+
+ test_cmp expect actual &&
+ test_cmp expect.tar actual.tar
+'
+
+test_expect_success 'GIT_DIR is set to the enclosing repo for ls-remote' '
+ git rev-parse --git-dir >expect &&
+ cmd=$(escape_spaces "echo \$GIT_DIR >actual && %S .") &&
+ git ls-remote "ext::sh -c $cmd" &&
+ test_cmp expect actual
+'
+
+test_expect_success 'GIT_DIR is set to the enclosing repo for archive' '
+ git rev-parse --git-dir >expect &&
+ git archive HEAD >expect.tar &&
+ cmd=$(escape_spaces "echo \$GIT_DIR >actual && %S .") &&
+ git archive --remote="ext::sh -c $cmd" HEAD >actual.tar &&
+ test_cmp expect actual &&
+ test_cmp expect.tar actual.tar
+'
+
+test_expect_success 'GIT_DIR is not set if there is no enclosing repo' '
+ rm -rf subdir &&
+ >actual &&
+ printf "%s\n" unset unset >expect &&
+ git archive HEAD >expect.tar &&
+
+ mkdir subdir &&
+ cmd=$(escape_spaces "echo \${GIT_DIR-unset} >>../actual && %S ..") &&
+ (
+ GIT_CEILING_DIRECTORIES=$(pwd) &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd subdir &&
+ git ls-remote "ext::sh -c $cmd" &&
+ git archive --remote="ext::sh -c $cmd" HEAD >../actual.tar
+ ) &&
+ test_cmp expect actual &&
+ test_cmp expect.tar actual.tar
+'
test_expect_success 'set up fake git-daemon' '
mkdir remote &&
diff --git a/transport-helper.c b/transport-helper.c
index 91aed35ebb..e4fd982383 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
helper->git_cmd = 0;
helper->silent_exec_failure = 1;
- argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
- get_git_dir());
+ if (have_git_dir())
+ argv_array_pushf(&helper->env_array, "%s=%s",
+ GIT_DIR_ENVIRONMENT, get_git_dir());
code = start_command(helper);
if (code < 0 && errno == ENOENT)
--
2.11.0.355.g1ede815
^ permalink raw reply related
* Re: [PATCH v2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR
From: Stefan Beller @ 2016-12-30 0:37 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jeff King, git@vger.kernel.org, Duy Nguyen, Junio C Hamano
In-Reply-To: <20161230001114.GB7883@aiede.mtv.corp.google.com>
> + mkdir lsremote-root &&
> + (
> + GIT_CEILING_DIRECTORIES=$(pwd) &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd lsremote-root &&
> + git ls-remote "$HTTPD_URL/smart/repo.git" >../actual
> + ) &&
We could avoid the subshell via
GIT_CEILING_DIRECTORIES=$(pwd) \
git -C lsremote-root lsremote ... >actual
Not sure if it is worth to trade off a block of code (and an extra shell
at run time) for an overly long line.
The rest looks good to me.
Thanks,
Stefan
^ permalink raw reply
* [PATCH] submodule.c: use GIT_DIR_ENVIRONMENT consistently
From: Stefan Beller @ 2016-12-30 0:47 UTC (permalink / raw)
To: gitster, jrnieder; +Cc: git, Stefan Beller
In C code we have the luxury of having constants for all the important
things that are hard coded. This is the only place in C, that hard codes
the git directory environment variable, so fix it.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Signed-off-by-the-format-patch-config ;)
This is the only occurrence for "GIT_DIR=" in C, but what about ".git"
git grep "\.git\"" *.c finds some places, which we may want to convert
to DEFAULT_GIT_DIR_ENVIRONMENT?
(mainly things that are newer if I can judge the places correctly
lots of submodules, worktrees and the no data in ".git" bug AFAICT)
Thanks,
Stefan
submodule.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/submodule.c b/submodule.c
index ece17315d6..973b9f3f96 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1333,5 +1333,6 @@ void prepare_submodule_repo_env(struct argv_array *out)
if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
argv_array_push(out, *var);
}
- argv_array_push(out, "GIT_DIR=.git");
+ argv_array_push(out, "%s=%s", GIT_DIR_ENVIRONMENT,
+ DEFAULT_GIT_DIR_ENVIRONMENT);
}
--
2.11.0.259.ga95e92af08.dirty
^ permalink raw reply related
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