Re: [RFC PATCH v2 2/4] git-credential-netrc: minor whitespace cleanup in test script
On Tue, Jun 12, 2018 at 11:10 PM, Todd Zullinger wrote: > Signed-off-by: Todd Zullinger > --- > diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh > b/contrib/credential/netrc/t-git-credential-netrc.sh > index 58191a62f8..c5661087fe 100755 > --- a/contrib/credential/netrc/t-git-credential-netrc.sh > +++ b/contrib/credential/netrc/t-git-credential-netrc.sh > @@ -17,15 +17,15 @@ > # set up test repository > > test_expect_success \ > -'set up test repository' \ > -'git config --add gpg.program test.git-config-gpg' > + 'set up test repository' \ > + 'git config --add gpg.program test.git-config-gpg' Since you're touching all the tests in this script anyhow, perhaps modernize them so the title and opening quote of the test body are on the same line as test_expect_success, and the closing body quote is on a line of its own? test_expect_sucess 'setup test repository' ' ...test body... ' I also changed "set up" to "setup" to follow existing practice. (Not necessarily worth a re-roll.) > # The external test will outputs its own plan > test_external_has_tap=1 > > test_external \ > -'git-credential-netrc' \ > -perl "$TEST_DIRECTORY"/../contrib/credential/netrc/test.pl > + 'git-credential-netrc' \ > + perl "$TEST_DIRECTORY"/../contrib/credential/netrc/test.pl > > test_done > )
[PATCH] doc: update the order of the syntax `git merge --continue`
The syntax "git merge HEAD " has been removed. The
order of the syntax should also be updated.
---
Documentation/git-merge.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index d5dfd8430..6a5c00e2c 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -57,7 +57,7 @@ reconstruct the original (pre-merge) changes. Therefore:
discouraged: while possible, it may leave you in a state that is hard to
back out of in the case of a conflict.
-The fourth syntax ("`git merge --continue`") can only be run after the
+The third syntax ("`git merge --continue`") can only be run after the
merge has resulted in conflicts.
OPTIONS
--
2.15.1 (Apple Git-101)
[RFC PATCH v2 3/4] git-credential-netrc: use in-tree Git.pm for tests
From: Luis Marsano
The netrc test.pl script calls git-credential-netrc which imports the
Git module. Pass GITPERLLIB to git-credential-netrc via PERL5LIB to
ensure the in-tree Git module is used for testing.
Signed-off-by: Luis Marsano
---
contrib/credential/netrc/t-git-credential-netrc.sh | 3 ++-
contrib/credential/netrc/test.pl | 1 -
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh
b/contrib/credential/netrc/t-git-credential-netrc.sh
index c5661087fe..07227d0228 100755
--- a/contrib/credential/netrc/t-git-credential-netrc.sh
+++ b/contrib/credential/netrc/t-git-credential-netrc.sh
@@ -23,9 +23,10 @@
# The external test will outputs its own plan
test_external_has_tap=1
+ export PERL5LIB="$GITPERLLIB"
test_external \
'git-credential-netrc' \
- perl "$TEST_DIRECTORY"/../contrib/credential/netrc/test.pl
+ perl "$GIT_BUILD_DIR"/contrib/credential/netrc/test.pl
test_done
)
diff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl
index 1e1001030e..2b5280ad6a 100755
--- a/contrib/credential/netrc/test.pl
+++ b/contrib/credential/netrc/test.pl
@@ -1,5 +1,4 @@
#!/usr/bin/perl
-use lib (split(/:/, $ENV{GITPERLLIB}));
use warnings;
use strict;
[RFC PATCH v2 4/4] git-credential-netrc: fix exit status when tests fail
From: Luis Marsano
Signed-off-by: Luis Marsano
---
contrib/credential/netrc/test.pl | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl
index 2b5280ad6a..c0fb3718b2 100755
--- a/contrib/credential/netrc/test.pl
+++ b/contrib/credential/netrc/test.pl
@@ -11,7 +11,6 @@ BEGIN
# t-git-credential-netrc.sh kicks off our testing, so we have to go
# from there.
Test::More->builder->current_test(1);
- Test::More->builder->no_ending(1);
}
my @global_credential_args = @ARGV;
@@ -103,6 +102,9 @@ BEGIN
ok(scalar keys %$cred == 2, 'Got keys decrypted by command option');
+my $is_passing = eval { Test::More->is_passing };
+exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;
+
sub run_credential
{
my $args = shift @_;
[RFC PATCH v2 0/4] contrib/credential/netrc Makefile & test improvements
This replaces my 2/2 "use in-tree Git.pm for tests" with Luis's improved version. It also adds Luis's fix to ensure the proper exit status on test failures and a minor whitespace cleanup. Is it alright to forge your signoff Luis? Luis Marsano (2): git-credential-netrc: use in-tree Git.pm for tests git-credential-netrc: fix exit status when tests fail Todd Zullinger (2): git-credential-netrc: make "all" default target of Makefile git-credential-netrc: minor whitespace cleanup in test script contrib/credential/netrc/Makefile | 3 +++ contrib/credential/netrc/t-git-credential-netrc.sh | 9 + contrib/credential/netrc/test.pl | 5 +++-- 3 files changed, 11 insertions(+), 6 deletions(-)
[RFC PATCH v2 1/4] git-credential-netrc: make "all" default target of Makefile
Running "make" in contrib/credential/netrc should run the "all" target rather than the "test" target. Add an empty "all::" target like most of our other Makefiles. Signed-off-by: Todd Zullinger --- contrib/credential/netrc/Makefile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contrib/credential/netrc/Makefile b/contrib/credential/netrc/Makefile index 0ffa407191..6174e3bb83 100644 --- a/contrib/credential/netrc/Makefile +++ b/contrib/credential/netrc/Makefile @@ -1,3 +1,6 @@ +# The default target of this Makefile is... +all:: + test: ./t-git-credential-netrc.sh
[RFC PATCH v2 2/4] git-credential-netrc: minor whitespace cleanup in test script
Signed-off-by: Todd Zullinger --- contrib/credential/netrc/t-git-credential-netrc.sh | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh b/contrib/credential/netrc/t-git-credential-netrc.sh index 58191a62f8..c5661087fe 100755 --- a/contrib/credential/netrc/t-git-credential-netrc.sh +++ b/contrib/credential/netrc/t-git-credential-netrc.sh @@ -17,15 +17,15 @@ # set up test repository test_expect_success \ -'set up test repository' \ -'git config --add gpg.program test.git-config-gpg' + 'set up test repository' \ + 'git config --add gpg.program test.git-config-gpg' # The external test will outputs its own plan test_external_has_tap=1 test_external \ -'git-credential-netrc' \ -perl "$TEST_DIRECTORY"/../contrib/credential/netrc/test.pl + 'git-credential-netrc' \ + perl "$TEST_DIRECTORY"/../contrib/credential/netrc/test.pl test_done )
Re: [RFC PATCH 0/2] contrib/credential/netrc Makefile & test improvements
Hi, Ævar Arnfjörð Bjarmason wrote: > > On Sun, Jun 10 2018, Todd Zullinger wrote: > >>> I added 'use autodie;' without realizing it had external dependencies. >>> According to the documentation >>> http://perldoc.perl.org/autodie.html >>> it's a pragma since perl 5.10.1 >>> Removing 'use autodie;' should be fine: it's not critical. >> >> I should clarify that part of why autodie isn't in my build >> environment is that the Fedora and RHEL7+ perl packages >> split out many modules which are shipped as part of the core >> perl tarball. So while all the platforms I care about have >> perl >= 5.10.1, the Fedora and newer RHEL systems have the >> autodie module in a separate package. >> >> That said, the INSTALL docs still suggest that we only >> require perl >= 5.8, so if autodie is easily removed, that >> would probably be a good plan. > > The intent of those docs was and still is to say "5.8 and the modules it > ships with". > > This was discussed when 2.17.0 was released with my changes to make us > unconditionally use Digest::MD5: > https://public-inbox.org/git/87fu50e0ht@evledraar.gmail.com/ > > As noted there that's not some dogmatic "RedHat altered perl so we don't > care about them", but rather that in practice this doesn't impact users > negatively, so I think it's fine. Yeah, that was my understanding. I only included the information on why it was missing from my build environment despite having perl-5.10.1 was due to the Fedora/Red Hat packaging. I agree that any issues which fall out of those packaging differences are on Fedora/Red Hat packagers to fix. (Which should all be relatively trivial, as the perl modules contain a 'Provides: perl($module)'. That allows dnf|yum install 'perl(autodie)' to easily pull in the right package. And the rpm perl dep generator creates a 'Requires: perl(autodie)' when is sees 'use autodie'.) Sorry if I muddied the conversation with that tangential info. :) >> Ævar brought up bumping the minimum supported perl to 5.10.0 >> last December, in <20171223174400.26668-1-ava...@gmail.com> >> (https://public-inbox.org/git/20171223174400.26668-1-ava...@gmail.com/). >> The general consensus seemed positive, but I don't think >> it's happened. Even so, that was 5.10.0, not the 5.10.1 >> which added autodie. > > Right, this doesn't apply to autodie. Looking at > https://www.cpan.org/ports/binaries.html there were a lot of releases > that had 5.10.0, not *.1. > > Also git-credential-netrc is in contrib, I don't know if that warrants > treating it differently, I don't use it myself, and don't know how much > it's "not really contrib" in practice (e.g. like the bash completion > which is installed everywhere...)> Indeed, that's a fine question. All the platforms I care about have 5.10.1 or newer, so either way works for me. > But yeah, skimming the code it would be easy to remove the dependency. I wrapped up the changes from Luis which replace my 2/2 "use in-tree Git.pm for tests" and to ensure the tests exit properly on failures. I'll send those out now in the hope that it saves a little effort in moving these minor fixes forward. As far as removing the autodie dep, is there anything more to that than dropping the 'use autodie' line? It looks like doing so leaves us no worse than we were before, but I haven't written any perl in a long time. Thanks, -- Todd ~~ Ocean, n. A body of water occupying about two-thirds of a world made for man -- who has no gills. -- Ambrose Bierce, "The Devil's Dictionary"
Re: [PATCH] config.c: fix regression for core.safecrlf false
On Mon, Jun 11, 2018 at 06:46:34PM -0700, Anthony Sottile wrote: [] > Anything else for me to do here? (sorry! not super familiar with the process) Your patch has been picked up by Junio, and is currently merged into the "pu" branch (proposed updates): commit bc8ff8aec33836af3fefe1bcd3f533a1486b793f Merge: e69b544a38 6cb09125be Author: Junio C Hamano Date: Tue Jun 12 10:15:13 2018 -0700 Merge branch 'as/safecrlf-quiet-fix' into jch Fix for 2.17-era regression. * as/safecrlf-quiet-fix: config.c: fix regression for core.safecrlf false >From there, it will typically progress into next and master, unless reviewers come with comments and improvements. You can watch out for "What's cooking in git" messages here on the list to follow the progress. >From my experience it will end up in a Git release, but I don't know, if it will be 2.18 or a later one.
Re: [PATCH] packfile: Correct zlib buffer handling
Hi,
Sorry about the delay here (bit of a mix-up and didn't reply to the list).
(see inline )
On Sun, May 27, 2018 at 9:41 PM, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>> Duy Nguyen writes:
>>
>>> On Sun, May 27, 2018 at 1:57 AM, Junio C Hamano wrote:
(trimming)
>
> Specifically, I was worried about this assertion:
>
> Lets rely on the fact that the source buffer will only be fully
> consumed when the when the destination buffer is inflated to the
> correct size.
>
> which I think is the exact bad thinking that caused troubles for us
> in the past; isn't the explanation in 456cdf6e ("Fix loose object
> uncompression check.", 2007-03-19) relevant here?
>
> - stream.avail_out = size + 1;
> + stream.avail_out = size;
> ...
> stream.next_in = in;
> st = git_inflate(, Z_FINISH);
> if (!stream.avail_out)
> - break; /* the payload is larger than it should be */
> + break; /* done, st indicates if source fully consumed
> */
> curpos += stream.next_in - in;
> } while (st == Z_OK || st == Z_BUF_ERROR);
> git_inflate_end();
> if ((st != Z_STREAM_END) || stream.total_out != size) {
> free(buffer);
> return NULL;
> }
>
> With minimum stream.avail_out without slack, when !avail_out, i.e.
> when we fully filled the output buffer, it could be that we had
> correct input that deflates to the correct size, in which case we
> are happy---st would say Z_STREAM_END, we would leave the loop
> because it is neither OK nor BUF_ERROR, and total_out would report
> the size we expected. Or the input zlib stream may have ended with
> bytes that express "this concludes the stream", and the input bytes
> before that was sufficient to construct the original payload fully,
> and we may have just fed the bytes before that "this concludes the
> stream" to git_inflate().
>
> In such a case, we haven't consumed all the avail_in. We may
> already have all the correct output, i.e. !avail_out, but because we
> haven't consumed the "this concludes the stream", st is not
> STREAM_END in such a case.
If I understand correctly your concerned the avail_in is longer than
what is required to fill the output buffer..
I'm fairly sure that won't result in a Z_STREAM_END, as you rightfully
point out, but the loop _will_ terminate due to the output buffer
being full and then since its not Z_STREAM_END the
unpack_compressed_entry fails, as it should.
>
> Our existing while() loop, with one-byte slack in avail_out, would
> have let us continue and the next iteration of the loop would have
> consumed the input without producing any more output (i.e. avail_out
> would have been left to 1 in both of these final two rounds) and we
> would have exited the loop. After calling inflate_end(), we would
> have noticed STREAM_END and correct size and we would have been
> happy.
Your assuming that zlib will terminate with an error, but a fully
decompressed buffer, because it hasn't consumed the entire input
buffer. I don't think that is how it works (its not how the
documentation is written, nor the bits of code i've looked at seem to
work, which granted i'm not a zlib maintainer).
>
> The updated code would handle this latter case rather badly, no? We
> leave the loop early, notice st is not STREAM_END, and be very
> unhappy, because this patch did not give us to consume the very end
> of the input stream and left the loop early.
Your correct if the above case is a valid zlib behavior then there
would be a problem. But, I don't think the termination is dicated by
insufficient output space until there is an attempt to utilize that
space.
>
>>> This yields two problems, first a single byte overrun won't be detected
>>> properly because the Z_STREAM_END will then be set, but the null
>>> terminator will have been overwritten.
>
> Because we compare total_out and size at the end, we would detect it
> as an error in this function, no? Then zlib overwriting NUL would
> not be a problem, as we would free the buffer and return NULL, no?
>
>>> The other problem is that
>>> more recent zlib patches have been poisoning the unconsumed portions
>>> of the buffers which also overwrites the null, while correctly
>>> returning length and status.
>
> Isn't that a bug in zlib, though? Or do they do that deliberately?
>
> I think a workaround with lower impact would be to manually restore
> NUL at the end of the buffer.
I agree, just resetting the NULL its likely safer, and I will repost a
patch soon which if nothing else makes git more robust to errant zlib
behavior.
[RFC PATCH 0/3] submodules with no working tree shall unset core.worktree
The first patch teaches checkout/reset (with --recurse-submodules) to unset the core.worktree config when the new state of the superprojects working tree doesn't contain the submodules working tree. The last patch is teaching "git submodule deinit" to unset the core.worktree setting as well. It turned out this one is tricky, as for that we also have to set it in the counter part, such as "submodule update". Thanks, Stefan Stefan Beller (3): submodule: unset core.worktree if no working tree is present submodule: ensure core.worktree is set after update submodule deinit: unset core.worktree builtin/submodule--helper.c | 26 ++ git-submodule.sh| 5 + submodule.c | 14 ++ submodule.h | 2 ++ t/lib-submodule-update.sh | 5 +++-- t/t7400-submodule-basic.sh | 5 + 6 files changed, 55 insertions(+), 2 deletions(-) -- 2.18.0.rc1.244.gcf134e6275-goog
[PATCH 2/3] submodule: ensure core.worktree is set after update
Signed-off-by: Stefan Beller
---
builtin/submodule--helper.c | 24
git-submodule.sh| 5 +
2 files changed, 29 insertions(+)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index bd250ca2164..dffc55ed8ee 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1860,6 +1860,29 @@ static int check_name(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int connect_gitdir_workingtree(int argc, const char **argv, const char
*prefix)
+{
+ struct strbuf sb = STRBUF_INIT;
+ const char *name, *path;
+ char *sm_gitdir;
+
+ if (argc != 3)
+ BUG("submodule--helper connect-gitdir-workingtree
");
+
+ name = argv[1];
+ path = argv[2];
+
+ strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
+ sm_gitdir = absolute_pathdup(sb.buf);
+
+ connect_work_tree_and_git_dir(path, sm_gitdir, 0);
+
+ strbuf_release();
+ free(sm_gitdir);
+
+ return 0;
+}
+
#define SUPPORT_SUPER_PREFIX (1<<0)
struct cmd_struct {
@@ -1873,6 +1896,7 @@ static struct cmd_struct commands[] = {
{"name", module_name, 0},
{"clone", module_clone, 0},
{"update-clone", update_clone, 0},
+ {"connect-gitdir-workingtree", connect_gitdir_workingtree, 0},
{"relative-path", resolve_relative_path, 0},
{"resolve-relative-url", resolve_relative_url, 0},
{"resolve-relative-url-test", resolve_relative_url_test, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index 78073cd87d1..6596a77c5ef 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -615,6 +615,11 @@ cmd_update()
die "$(eval_gettext "Unable to find current
\${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
fi
+ if ! $(git config -f "$(git rev-parse
--git-common-dir)/modules/$name/config" core.worktree) 2>/dev/null
+ then
+ git submodule--helper connect-gitdir-workingtree $name
$sm_path
+ fi
+
if test "$subsha1" != "$sha1" || test -n "$force"
then
subforce=$force
--
2.18.0.rc1.244.gcf134e6275-goog
[PATCH 1/3] submodule: unset core.worktree if no working tree is present
When a submodules work tree is removed, we should unset its core.worktree
setting as the worktree is no longer present. This is not just in line
with the conceptual view of submodules, but it fixes an inconvenience
for looking at submodules that are not checked out:
git clone --recurse-submodules git://github.com/git/git && cd git &&
git checkout --recurse-submodules v2.13.0
git -C .git/modules/sha1collisiondetection log
fatal: cannot chdir to '../../../sha1collisiondetection': \
No such file or directory
With this patch applied, the final call to git log works instead of dying
in its setup, as the checkout will unset the core.worktree setting such
that following log will be run in a bare repository.
This patch covers all commands that are in the unpack machinery, i.e.
checkout, read-tree, reset. A follow up patch will address
"git submodule deinit", which will also make use of the new function
submodule_unset_core_worktree(), which is why we expose it in this patch.
Signed-off-by: Stefan Beller
---
submodule.c | 14 ++
submodule.h | 2 ++
t/lib-submodule-update.sh | 3 ++-
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/submodule.c b/submodule.c
index 939d6870ecd..e127c074b04 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1532,6 +1532,18 @@ int bad_to_remove_submodule(const char *path, unsigned
flags)
return ret;
}
+void submodule_unset_core_worktree(const struct submodule *sub)
+{
+ char *config_path = xstrfmt("%s/modules/%s/config",
+ get_git_common_dir(), sub->name);
+
+ if (git_config_set_in_file_gently(config_path, "core.worktree", NULL))
+ warning(_("Could not unset core.worktree setting in submodule
'%s'"),
+ sub->path);
+
+ free(config_path);
+}
+
static const char *get_super_prefix_or_empty(void)
{
const char *s = get_super_prefix();
@@ -1697,6 +1709,8 @@ int submodule_move_head(const char *path,
if (is_empty_dir(path))
rmdir_or_warn(path);
+
+ submodule_unset_core_worktree(sub);
}
}
out:
diff --git a/submodule.h b/submodule.h
index 7856b8a0b3d..4644683e6cb 100644
--- a/submodule.h
+++ b/submodule.h
@@ -121,6 +121,8 @@ extern int submodule_move_head(const char *path,
const char *new_head,
unsigned flags);
+void submodule_unset_core_worktree(const struct submodule *sub);
+
/*
* Prepare the "env_array" parameter of a "struct child_process" for executing
* a submodule by clearing any repo-specific environment variables, but
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 1f38a85371a..12cd4e9233e 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -709,7 +709,8 @@ test_submodule_recursing_with_args_common() {
git branch -t remove_sub1 origin/remove_sub1 &&
$command remove_sub1 &&
test_superproject_content origin/remove_sub1 &&
- ! test -e sub1
+ ! test -e sub1 &&
+ test_must_fail git config -f .git/modules/sub1/config
core.worktree
)
'
# ... absorbing a .git directory along the way.
--
2.18.0.rc1.244.gcf134e6275-goog
[PATCH 3/3] submodule deinit: unset core.worktree
When a submodule is deinit'd, the working tree is gone, so the setting of
core.worktree is bogus. Unset it.
Signed-off-by: Stefan Beller
---
builtin/submodule--helper.c | 2 ++
t/lib-submodule-update.sh | 2 +-
t/t7400-submodule-basic.sh | 5 +
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index dffc55ed8ee..19480902681 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -980,6 +980,8 @@ static void deinit_submodule(const char *path, const char
*prefix,
if (!(flags & OPT_QUIET))
printf(format, displaypath);
+ submodule_unset_core_worktree(sub);
+
strbuf_release(_rm);
}
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 12cd4e9233e..aa5ac03325a 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -235,7 +235,7 @@ reset_work_tree_to_interested () {
then
mkdir -p submodule_update/.git/modules/sub1/modules &&
cp -r submodule_update_repo/.git/modules/sub1/modules/sub2
submodule_update/.git/modules/sub1/modules/sub2
- GIT_WORK_TREE=. git -C
submodule_update/.git/modules/sub1/modules/sub2 config --unset core.worktree
+ # core.worktree is unset for sub2 as it is not checked out
fi &&
# indicate we are interested in the submodule:
git -C submodule_update config submodule.sub1.url "bogus" &&
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index be946fb1fd1..c6e1f749639 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -993,6 +993,11 @@ test_expect_success 'submodule deinit should remove the
whole submodule section
rmdir init
'
+test_expect_success 'submodule deinit should unset core.worktree' '
+ test_path_is_file .git/modules/example/config &&
+ test_must_fail git config -f .git/modules/example/config core.worktree
+'
+
test_expect_success 'submodule deinit from subdirectory' '
git submodule update --init &&
git config submodule.example.foo bar &&
--
2.18.0.rc1.244.gcf134e6275-goog
[PATCH] RelNotes 2.18: clarify where directory rename detection applies
Mention that this feature works with some commands (merge and cherry-pick, implying that it also works with commands that build on these like rebase -m and rebase -i). Explicitly mentioning two commands hopefully implies that it may not always work with other commands (am, and rebase without flags that imply either -m or -i). Also, since the directory rename detection from this cycle was specifically added in merge-recursive and not diffcore-rename, remove the 'in "diff" family" phrase from the note. (Folks have requested in the past that `git diff` detect directory renames and somehow simplify its output, so it may be helpful to avoid implying that diff has any new capability here.) Signed-off-by: Elijah Newren --- After thinking for a while about my RFC at https://public-inbox.org/git/cabpp-bf4gbwvrha3d1vqxusnh3as9xvlqteuemfmldkpccy...@mail.gmail.com/ this commit seems like a simple fix. We can discuss during the 2.19 and 2.20 cycles what to do with rebase, if anything. Also, if the above commit message feels incomplete without an explanation of why directory rename detection doesn't work with git-am, the following could be included: More details about the git-am limitation for the curious: git-am tries to avoid a full three way merge, instead calling git-apply. That prevents us from detecting renames at all, which may defeat the directory rename detection. There is a fallback, though; if the initial git-apply fails and the user has specified the -3 option, git-am will fall back to a three way merge. However, git-am lacks the necessary information to do a "real" three way merge. Instead, it has to use build_fake_ancestor() to get a merge base that is missing files whose rename may have been important to detect for directory rename detection to function. am-based rebases work by first generating a bunch of patches (which no longer record what the original commits were and thus don't have the necessary info from which we can find a real merge-base), and then calling `git am`. This implies that am-based rebases will not always successfully detect directory renames either. merged-based rebases (rebase -m) and cherry-pick-based rebases (rebase -i) are not affected by this shortcoming. Documentation/RelNotes/2.18.0.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/RelNotes/2.18.0.txt b/Documentation/RelNotes/2.18.0.txt index ed80e5485b..449e49e0eb 100644 --- a/Documentation/RelNotes/2.18.0.txt +++ b/Documentation/RelNotes/2.18.0.txt @@ -6,7 +6,7 @@ Updates since v2.17 UI, Workflows & Features - * Rename detection logic in "diff" family that is used in "merge" has + * Rename detection logic that is used in "merge" and "cherry-pick" has learned to guess when all of x/a, x/b and x/c have moved to z/a, z/b and z/c, it is likely that x/d added in the meantime would also want to move to z/d by taking the hint that the entire directory -- 2.18.0.rc0.3.gda9bce4c68
Re: `git merge --abort` does not run `git rerere clear`
On 12/06/2018, Junio C Hamano wrote: > Sam Kuper writes: >> [...] It makes sense that `git am [--skip|--abort]` and `git rebase >> [--skip|--abort]` would run `git rerere clear`. >> >> However, if they run it, then shouldn't `git merge --abort` run it, too? >> >> If not, then what is the reason why not [...] > > I do not think there was any reason, other than that those who added > "git merge --abort" weren't as careful as they should have been ;-) Thanks, good to know. > The command is a mere synonym to "git reset --merge"; Indeed it seems so. Thank you for pointing this out. > I am not so > confident that "git reset --merge" should also clear the current > rerere state. If (and this is a big if) "git reset --merge" should, > probably the right place to do so would be remove_branch_state(), > before the function removes merge_rr file. Unfortunately, I am still not familiar enough with the Git codebase to be able to express an informed opinion about this. Sorry :( > Doing so might allow us > to lose calls to rerere_clear() individual codepaths of various > "abort" implementations make, That, I think, was an example of a garden path sentence.[1] Took me more than one parse to understand it :) Anyhow, yes, I agree that this might be an opportunity to DRY the codebase in that regard. (And this would be a good thing, if so.) > but that would certainly require > careful thinking to avoid unintended regressions. I don't use `git reset --merge` often enough to have formed an opinion about whether there are any use-cases for it in which it would be inappropriate for it to run `git rerere clear`. Apologies again not to be able to be more helpful. I hope that you or others on the list will be able to consider this matter, and the question of how/where to best implement the change. Thank you for your work maintaining Git! [1] https://en.wikipedia.org/wiki/Garden_path_sentence
Re: [PATCHv2 0/6] git-p4: some small fixes updated
On 12 June 2018 at 22:53, Eric Sunshine wrote: > On Tue, Jun 12, 2018 at 5:49 PM, Luke Diamand wrote: >> Thanks. While on the subject of git-p4, I thought I should mention >> that I've been looking at getting git-p4 to work with Python3. >> >> I've got some low risk easy (mostly automated) changes which get it to >> the point where it compiles. After that I have to figure out how to >> fix the byte-vs-string unicode problem that Python3 brings. [...] > > See how reposurgeon handles the problem[1]. It defines polystr() and > polybytes() functions which coerce strings and byte sequences as > needed. It's not pretty, but it works. > > [1]: https://gitlab.com/esr/reposurgeon/blob/master/reposurgeon#L100 Thanks, that's got some useful tips!
Re: [PATCHv2 0/6] git-p4: some small fixes updated
On Tue, Jun 12, 2018 at 5:49 PM, Luke Diamand wrote: > Thanks. While on the subject of git-p4, I thought I should mention > that I've been looking at getting git-p4 to work with Python3. > > I've got some low risk easy (mostly automated) changes which get it to > the point where it compiles. After that I have to figure out how to > fix the byte-vs-string unicode problem that Python3 brings. [...] See how reposurgeon handles the problem[1]. It defines polystr() and polybytes() functions which coerce strings and byte sequences as needed. It's not pretty, but it works. [1]: https://gitlab.com/esr/reposurgeon/blob/master/reposurgeon#L100
Re: [PATCHv2 0/6] git-p4: some small fixes updated
On 12 June 2018 at 22:35, Junio C Hamano wrote: > Luke Diamand writes: > >> On 12 June 2018 at 18:10, Junio C Hamano wrote: >>> Luke Diamand writes: >>> This is an updated version of the set of changes I posted recently, following comments on the list: disable automatic sync after git-p4 submit: https://marc.info/?l=git=152818734814838=2 better handling of being logged out by Perforce: https://marc.info/?l=git=152818893815326=2 adapt the block size automatically on git-p4 submit: https://marc.info/?l=git=152819004315688=2 - Spelling corrections (Eric) - Improved comments (Eric) - Exception class hierarchy fix (Merland) - test simplification (Eric) >>> >>> That reminds me of one thing. >>> >>> This 6-patch series depends on the rm/p4-submit-with-commit-option >>> that came without and still waiting for a sign-off by the original >>> author. Also I do not think the original patch reached the public >>> list, so I'm attaching the patch to make sure people know which >>> patch I am talking about. >>> >>> Romain, can we get your sign-off on the patch you sent earlier? >> >> Wasn't it already sent in this message: >> >> https://marc.info/?l=git=152783923418317=2 >> >> Luke > > > Thanks for a pointer. Will replace and requeue. Thanks. While on the subject of git-p4, I thought I should mention that I've been looking at getting git-p4 to work with Python3. I've got some low risk easy (mostly automated) changes which get it to the point where it compiles. After that I have to figure out how to fix the byte-vs-string unicode problem that Python3 brings. Having been playing around with it, I think it should be possible to make the same git-p4.py script work with 2.7, 3.6 and probably 2.6, although I'm still some way from making this last bit work. Luke
Re: [PATCH] t7400: encapsulate setup code in test_expect_success
On Tue, Jun 12, 2018 at 5:25 PM, Stefan Beller wrote: > When running t7400 in a shell you observe more output than expected: > ... > ok 10 - submodule add > [master (root-commit) d79ce16] one > Author: A U Thor > 1 file changed, 1 insertion(+) > create mode 100644 one.t > ok 11 - redirected submodule add does not show progress > ... > Fix the output by encapsulating the setup code in test_expect_success > > Signed-off-by: Stefan Beller > --- > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > @@ -126,8 +126,10 @@ test_expect_success 'submodule add' ' > -test_create_repo parent && > -test_commit -C parent one > +test_expect_success 'setup parent and one repository for further tests' ' Nit: "for further tests" is implied for actions performed by a "setup" function, so a bit redundant to say so. > + test_create_repo parent && > + test_commit -C parent one > +'
Re: [PATCHv2 0/6] git-p4: some small fixes updated
Luke Diamand writes: > On 12 June 2018 at 18:10, Junio C Hamano wrote: >> Luke Diamand writes: >> >>> This is an updated version of the set of changes I posted recently, >>> following comments on the list: >>> >>> disable automatic sync after git-p4 submit: >>> https://marc.info/?l=git=152818734814838=2 >>> >>> better handling of being logged out by Perforce: >>>https://marc.info/?l=git=152818893815326=2 >>> >>> adapt the block size automatically on git-p4 submit: >>>https://marc.info/?l=git=152819004315688=2 >>> >>> - Spelling corrections (Eric) >>> - Improved comments (Eric) >>> - Exception class hierarchy fix (Merland) >>> - test simplification (Eric) >>> >> >> That reminds me of one thing. >> >> This 6-patch series depends on the rm/p4-submit-with-commit-option >> that came without and still waiting for a sign-off by the original >> author. Also I do not think the original patch reached the public >> list, so I'm attaching the patch to make sure people know which >> patch I am talking about. >> >> Romain, can we get your sign-off on the patch you sent earlier? > > Wasn't it already sent in this message: > > https://marc.info/?l=git=152783923418317=2 > > Luke Thanks for a pointer. Will replace and requeue.
[PATCH] t7400: encapsulate setup code in test_expect_success
When running t7400 in a shell you observe more output than expected: ... ok 8 - setup - hide init subdirectory ok 9 - setup - repository to add submodules to ok 10 - submodule add [master (root-commit) d79ce16] one Author: A U Thor 1 file changed, 1 insertion(+) create mode 100644 one.t ok 11 - redirected submodule add does not show progress ok 12 - redirected submodule add --progress does show progress ok 13 - submodule add to .gitignored path fails ... Fix the output by encapsulating the setup code in test_expect_success Signed-off-by: Stefan Beller --- t/t7400-submodule-basic.sh | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 1a33040d94d..c6e1f749639 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -126,8 +126,10 @@ test_expect_success 'submodule add' ' test_cmp empty untracked ' -test_create_repo parent && -test_commit -C parent one +test_expect_success 'setup parent and one repository for further tests' ' + test_create_repo parent && + test_commit -C parent one +' test_expect_success 'redirected submodule add does not show progress' ' git -C addtest submodule add "file://$submodurl/parent" submod-redirected \ -- 2.18.0.rc1.244.gcf134e6275-goog
Re: [PATCHv2 0/6] git-p4: some small fixes updated
On 12 June 2018 at 18:10, Junio C Hamano wrote:
> Luke Diamand writes:
>
>> This is an updated version of the set of changes I posted recently,
>> following comments on the list:
>>
>> disable automatic sync after git-p4 submit:
>> https://marc.info/?l=git=152818734814838=2
>>
>> better handling of being logged out by Perforce:
>>https://marc.info/?l=git=152818893815326=2
>>
>> adapt the block size automatically on git-p4 submit:
>>https://marc.info/?l=git=152819004315688=2
>>
>> - Spelling corrections (Eric)
>> - Improved comments (Eric)
>> - Exception class hierarchy fix (Merland)
>> - test simplification (Eric)
>>
>
> That reminds me of one thing.
>
> This 6-patch series depends on the rm/p4-submit-with-commit-option
> that came without and still waiting for a sign-off by the original
> author. Also I do not think the original patch reached the public
> list, so I'm attaching the patch to make sure people know which
> patch I am talking about.
>
> Romain, can we get your sign-off on the patch you sent earlier?
Wasn't it already sent in this message:
https://marc.info/?l=git=152783923418317=2
Luke
>
> Thanks.
>
> -- >8 --
> From: Romain Merland
> Date: Wed, 9 May 2018 17:32:12 +0200
> Subject: [PATCH] git-p4: add options --commit and --disable-rebase
>
> On a daily work with multiple local git branches, the usual way to
> submit only a specified commit was to cherry-pick the commit on
> master then run git-p4 submit. It can be very annoying to switch
> between local branches and master, only to submit one commit. The
> proposed new way is to select directly the commit you want to
> submit.
>
> Add option --commit to command 'git-p4 submit' in order to submit
> only specified commit(s) in p4.
>
> On a daily work developping software with big compilation time, one
> may not want to rebase on his local git tree, in order to avoid long
> recompilation.
>
> Add option --disable-rebase to command 'git-p4 submit' in order to
> disable rebase after submission.
>
> Reviewed-by: Luke Diamand
> Signed-off-by: Junio C Hamano
> ---
> Documentation/git-p4.txt | 14 ++
> git-p4.py| 29 +++--
> t/t9807-git-p4-submit.sh | 40
> 3 files changed, 77 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index d8c8f11c9f..88d109debb 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -149,6 +149,12 @@ To specify a branch other than the current one, use:
> $ git p4 submit topicbranch
>
>
> +To specify a single commit or a range of commits, use:
> +
> +$ git p4 submit --commit
> +$ git p4 submit --commit
> +
> +
> The upstream reference is generally 'refs/remotes/p4/master', but can
> be overridden using the `--origin=` command-line option.
>
> @@ -330,6 +336,14 @@ These options can be used to modify 'git p4 submit'
> behavior.
> p4/master. See the "Sync options" section above for more
> information.
>
> +--commit |::
> +Submit only the specified commit or range of commits, instead of the full
> +list of changes that are in the current Git branch.
> +
> +--disable-rebase::
> +Disable the automatic rebase after all commits have been successfully
> +submitted.
> +
> Rebase options
> ~~
> These options can be used to modify 'git p4 rebase' behavior.
> diff --git a/git-p4.py b/git-p4.py
> index 7bb9cadc69..f4a6f3b4c3 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1352,7 +1352,12 @@ def __init__(self):
> optparse.make_option("--update-shelve",
> dest="update_shelve", action="append", type="int",
> metavar="CHANGELIST",
> help="update an existing shelved
> changelist, implies --shelve, "
> - "repeat in-order for multiple
> shelved changelists")
> + "repeat in-order for multiple
> shelved changelists"),
> +optparse.make_option("--commit", dest="commit",
> metavar="COMMIT",
> + help="submit only the specified
> commit(s), one commit or xxx..xxx"),
> +optparse.make_option("--disable-rebase",
> dest="disable_rebase", action="store_true",
> + help="Disable rebase after submit is
> completed. Can be useful if you "
> + "work from a local git branch that is
> not master")
> ]
> self.description = "Submit changes from git to the perforce depot."
> self.usage += " [name of git branch to submit into perforce depot]"
> @@ -1362,6 +1367,8 @@ def __init__(self):
> self.dry_run = False
> self.shelve = False
> self.update_shelve = list()
> +self.commit = ""
> +
Re: What's cooking in git.git (Jun 2018, #03; Tue, 12)
> A tl;dr summary for -rc2 that hopefully should happen in 24 hours: > the following four topics > > jk/submodule-fsck-loose-fixup > sb/submodule-merge-in-merge-recursive > jk/index-pack-maint > sg/completion-zsh-workaround > > are planned to be merged in -rc2. It would be nice to have https://public-inbox.org/git/20180609110414.ga5...@duynguyen.home/ included as well, but as it is not yet a proper patch, I can understand why we err on the cautious side.
What's cooking in git.git (Jun 2018, #03; Tue, 12)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. A tl;dr summary for -rc2 that hopefully should happen in 24 hours: the following four topics jk/submodule-fsck-loose-fixup sb/submodule-merge-in-merge-recursive jk/index-pack-maint sg/completion-zsh-workaround are planned to be merged in -rc2. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [New Topics] * ab/refspec-init-fix (2018-06-11) 3 commits - refspec: initalize `refspec_item` in `valid_fetch_refspec()` - refspec: add back a refspec_item_init() function - refspec: s/refspec_item_init/&_or_die/g Make refspec parsing codepath more robust. Will merge to 'next'. * as/safecrlf-quiet-fix (2018-06-11) 1 commit - config.c: fix regression for core.safecrlf false Fix for 2.17-era regression. Will merge to 'next'. * jk/submodule-fsck-loose-fixup (2018-06-11) 2 commits (merged to 'next' on 2018-06-11 at 3eadb39c0a) + fsck: avoid looking at NULL blob->object + t7415: don't bother creating commit for symlink test Finishing touches to a topic that already is in 'maint'. Will merge to 'master' and then to 'maint'. * sb/submodule-merge-in-merge-recursive (2018-06-11) 1 commit (merged to 'next' on 2018-06-11 at ad05b6bc6a) + merge-submodule: reduce output verbosity Finishing touches to a topic that already is in 'master'. Will merge to 'master'. * sg/completion-zsh-workaround (2018-06-12) 1 commit (merged to 'next' on 2018-06-12 at 331a1db143) + completion: correct zsh detection when run from git-completion.zsh Work around zsh segfaulting when loading git-completion.zsh Will merge to 'master'. * sg/gpg-tests-fix (2018-06-11) 2 commits - tests: make forging GPG signed commits and tags more robust - t7510-signed-commit: use 'test_must_fail' Some flaky tests have been fixed. Will merge to 'next'. * jk/fetch-all-peeled-fix (2018-06-11) 1 commit - fetch-pack: don't try to fetch peel values with --all * rd/diff-options-typofix (2018-06-11) 1 commit - diff-options.txt: fix minor typos, font inconsistencies, in docs * ld/git-p4-updates (2018-06-12) 6 commits - git-p4: auto-size the block - git-p4: narrow the scope of exceptions caught when parsing an int - git-p4: raise exceptions from p4CmdList based on error from p4 server - git-p4: better error reporting when p4 fails - git-p4: add option to disable syncing of p4/master with p4 - git-p4: disable-rebase: allow setting this via configuration (this branch uses rm/p4-submit-with-commit-option.) * en/merge-recursive-cleanup (2018-06-12) 6 commits - merge-recursive: add pointer about unduly complex looking code - merge-recursive: rename conflict_rename_*() family of functions - merge-recursive: clarify the rename_dir/RENAME_DIR meaning - merge-recursive: align labels with their respective code blocks - merge-recursive: fix numerous argument alignment issues - merge-recursive: fix miscellaneous grammar error in comment * jh/partial-clone (2018-06-12) 1 commit - list-objects: check if filter is NULL before using * km/doc-workflows-typofix (2018-06-12) 1 commit - gitworkflows: fix grammar in 'Merge upwards' rule * ms/send-pack-honor-config (2018-06-12) 1 commit - builtin/send-pack: populate the default configs -- [Stalled] * ab/fetch-tags-noclobber (2018-05-16) 9 commits - fixup! push tests: assert re-pushing annotated tags - fetch: stop clobbering existing tags without --force - fetch tests: add a test clobbering tag behavior - fetch tests: correct a comment "remove it" -> "remove them" - push doc: correct lies about how push refspecs work - push tests: assert re-pushing annotated tags - push tests: add more testing for forced tag pushing - push tests: fix logic error in "push" test assertion - push tests: remove redundant 'git push' invocation Expecting a reboot of the discussion to take it to some conclusion and then a reroll. cf. cf. cf. cf. * pw/add-p-select (2018-03-16) 3 commits - add -p: optimize line selection for short hunks - add -p: allow line selection to be inverted - add -p: select individual hunk lines "git add -p" interactive interface learned to let users choose individual added/removed lines to be used in the operation, instead of accepting or rejecting a whole hunk. Expecting a reroll to reignite the discussion. cf. <9895c7b7-eac4-28c1-90c6-443acd113...@talktalk.net> * jh/json-writer (2018-03-28) 1 commit - json_writer: new routines to create data in JSON format Preparatory code to later add json output for unspecified telemetry data. We do not add random code
Re: [PATCH v9] json_writer: new routines to create JSON data
g...@jeffhostetler.com writes:
> +static void indent_pretty(struct json_writer *jw)
> +{
> + int k;
> +
> + if (!jw->pretty)
> + return;
> +
> + for (k = 0; k < jw->open_stack.len; k++)
> + strbuf_addstr(>json, " ");
> +}
> +static void array_common(struct json_writer *jw)
> +{
> + assert_in_array(jw);
> + maybe_add_comma(jw);
> +
> + if (jw->pretty)
> + strbuf_addch(>json, '\n');
> + indent_pretty(jw);
Make it
if (jw->pretty) {
strbuf_addch(>json, '\n');
indent_pretty(jw);
}
and lose "be noop unless jw->pretty" check from the indent function.
Most other codepaths that cares about pretty printing state use that
"I check and decide to (or not to) call helpers that unconditionally
do" pattern, and the way indent_pretty() is used in the patch stands
out like a sore thumb.
Re: How to delete files and directories from git commit history?
Steve Litt writes: > But then I view all filenames from that directory that have ever been > in the project, as follows: > > git cat-file --buffer --batch-all-objects \ > --batch-check='%(objecttype) %(objectname)' \ > | grep ^c | cut -d " " -f 2 \ > | xargs -n 1 git ls-tree -r | sort | uniq \ > | grep propdir > > The preceding command lists directory docs/propdir and all the files > it's ever contained In which repository did you run this? As cat-file's documentation clearly states, --batch-all-objects does not *care* about the reachability, so even after rewriting the history using filter-branch, as long as you have the original objects before your history rewrite in the repository, it _will_ see those trees with propdir, even if they are now unreachable thanks to your filter-branch. If you are doing the above in the original (or in a local clone), try running "git repack -a -d && git prune --expire=now" after filter-branch.
Re: How to delete files and directories from git commit history?
On Tue, Jun 12, 2018 at 4:26 PM, Christian Couder wrote: > On Tue, Jun 12, 2018 at 9:44 PM, Steve Litt wrote: > >> My project (call it myproject) had a directory (call it docs/propdir) >> that was unnecessary for the project, and I've decided I don't want to >> offer the files in that directory as free software. So I need to delete >> docs/propdir from all commits in the repository. I did the following, >> while in my working repository's myproject directory: >> >> git filter-branch --tree-filter 'rm -rf docs/propdir' HEAD > >> What command do I do to remove all mention of doc/propdir and its >> files from my git history? > > Did you check the "CHECKLIST FOR SHRINKING A REPOSITORY" section of > the filter-branch man/help page? Also see BFG Repo Cleaner for an alternative. https://rtyley.github.io/bfg-repo-cleaner/
Re: [PATCH v9] json_writer: new routines to create JSON data
On Tue, Jun 12, 2018 at 4:22 PM, wrote:
> Add "struct json_writer" and a series of jw_ routines to compose JSON
> data into a string buffer. The resulting string may then be printed by
> commands wanting to support a JSON-like output format.
>
> The json_writer is limited to correctly formatting structured data for
> output. It does not attempt to build an object model of the JSON data.
>
> We say "JSON-like" because we do not enforce the Unicode (usually UTF-8)
> requirement on string fields. Internally, Git does not necessarily have
> Unicode/UTF-8 data for most fields, so it is currently unclear the best
> way to enforce that requirement. For example, on Linx pathnames can
s/Linx/Linux/
> contain arbitrary 8-bit character data, so a command like "status" would
> not know how to encode the reported pathnames. We may want to revisit
> this (or double encode such strings) in the future.
>
> Signed-off-by: Jeff Hostetler
> ---
> diff --git a/t/helper/test-json-writer.c b/t/helper/test-json-writer.c
> @@ -0,0 +1,564 @@
> +void get_s(int line_nr, char **s_in)
s/^/static/
> +{
> + *s_in = strtok(NULL, " ");
> + if (!*s_in)
> + die("line[%d]: expected: ", line_nr);
> +}
> +
> +void get_i(int line_nr, intmax_t *s_in)
s/^/static/
> +{
> + char *s;
> + char *endptr;
> +
> + get_s(line_nr, );
> +
> + *s_in = strtol(s, , 10);
> + if (*endptr || errno == ERANGE)
> + die("line[%d]: invalid integer value", line_nr);
> +}
> +
> +void get_d(int line_nr, double *s_in)
s/^/static/
> +{
> + char *s;
> + char *endptr;
> +
> + get_s(line_nr, );
> +
> + *s_in = strtod(s, );
> + if (*endptr || errno == ERANGE)
> + die("line[%d]: invalid float value", line_nr);
> +}
Re: How to delete files and directories from git commit history?
Hi, On Tue, Jun 12, 2018 at 9:44 PM, Steve Litt wrote: > My project (call it myproject) had a directory (call it docs/propdir) > that was unnecessary for the project, and I've decided I don't want to > offer the files in that directory as free software. So I need to delete > docs/propdir from all commits in the repository. I did the following, > while in my working repository's myproject directory: > > git filter-branch --tree-filter 'rm -rf docs/propdir' HEAD [...] > What command do I do to remove all mention of doc/propdir and its > files from my git history? Did you check the "CHECKLIST FOR SHRINKING A REPOSITORY" section of the filter-branch man/help page? Best, Christian.
Re: How to delete files and directories from git commit history?
On Tue, Jun 12, 2018 at 03:44:13PM -0400, Steve Litt wrote: > git filter-branch --tree-filter 'rm -rf docs/propdir' HEAD ... > What command do I do to remove all mention of doc/propdir and its > files from my git history? Are you sure that you pruned all branches? I would have expected a command like "git filter-branch ... -- --all" to prune every branch from the directories. Greetings, Eckhard
[PATCH v9] json_writer: new routines to create JSON data
From: Jeff Hostetler
Add "struct json_writer" and a series of jw_ routines to compose JSON
data into a string buffer. The resulting string may then be printed by
commands wanting to support a JSON-like output format.
The json_writer is limited to correctly formatting structured data for
output. It does not attempt to build an object model of the JSON data.
We say "JSON-like" because we do not enforce the Unicode (usually UTF-8)
requirement on string fields. Internally, Git does not necessarily have
Unicode/UTF-8 data for most fields, so it is currently unclear the best
way to enforce that requirement. For example, on Linx pathnames can
contain arbitrary 8-bit character data, so a command like "status" would
not know how to encode the reported pathnames. We may want to revisit
this (or double encode such strings) in the future.
Helped-by: Eric Sunshine
Helped-by: René Scharfe
Helped-by: Wink Saville
Helped-by: Ramsay Jones
Signed-off-by: Jeff Hostetler
---
Makefile| 2 +
json-writer.c | 414
json-writer.h | 105 +
t/helper/test-json-writer.c | 564
t/helper/test-tool.c| 1 +
t/helper/test-tool.h| 1 +
t/t0019-json-writer.sh | 331 ++
t/t0019/parse_json.perl | 52
8 files changed, 1470 insertions(+)
create mode 100644 json-writer.c
create mode 100644 json-writer.h
create mode 100644 t/helper/test-json-writer.c
create mode 100755 t/t0019-json-writer.sh
create mode 100644 t/t0019/parse_json.perl
diff --git a/Makefile b/Makefile
index 1d27f36..5a781e2 100644
--- a/Makefile
+++ b/Makefile
@@ -709,6 +709,7 @@ TEST_BUILTINS_OBJS += test-example-decorate.o
TEST_BUILTINS_OBJS += test-genrandom.o
TEST_BUILTINS_OBJS += test-hashmap.o
TEST_BUILTINS_OBJS += test-index-version.o
+TEST_BUILTINS_OBJS += test-json-writer.o
TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o
TEST_BUILTINS_OBJS += test-match-trees.o
TEST_BUILTINS_OBJS += test-mergesort.o
@@ -871,6 +872,7 @@ LIB_OBJS += hashmap.o
LIB_OBJS += help.o
LIB_OBJS += hex.o
LIB_OBJS += ident.o
+LIB_OBJS += json-writer.o
LIB_OBJS += kwset.o
LIB_OBJS += levenshtein.o
LIB_OBJS += line-log.o
diff --git a/json-writer.c b/json-writer.c
new file mode 100644
index 000..9c79aa9
--- /dev/null
+++ b/json-writer.c
@@ -0,0 +1,414 @@
+#include "cache.h"
+#include "json-writer.h"
+
+void jw_init(struct json_writer *jw)
+{
+ strbuf_reset(>json);
+ strbuf_reset(>open_stack);
+ jw->need_comma = 0;
+ jw->pretty = 0;
+}
+
+void jw_release(struct json_writer *jw)
+{
+ strbuf_release(>json);
+ strbuf_release(>open_stack);
+}
+
+/*
+ * Append JSON-quoted version of the given string to 'out'.
+ */
+static void append_quoted_string(struct strbuf *out, const char *in)
+{
+ unsigned char c;
+
+ strbuf_addch(out, '"');
+ while ((c = *in++) != '\0') {
+ if (c == '"')
+ strbuf_addstr(out, "\\\"");
+ else if (c == '\\')
+ strbuf_addstr(out, "");
+ else if (c == '\n')
+ strbuf_addstr(out, "\\n");
+ else if (c == '\r')
+ strbuf_addstr(out, "\\r");
+ else if (c == '\t')
+ strbuf_addstr(out, "\\t");
+ else if (c == '\f')
+ strbuf_addstr(out, "\\f");
+ else if (c == '\b')
+ strbuf_addstr(out, "\\b");
+ else if (c < 0x20)
+ strbuf_addf(out, "\\u%04x", c);
+ else
+ strbuf_addch(out, c);
+ }
+ strbuf_addch(out, '"');
+}
+
+static void indent_pretty(struct json_writer *jw)
+{
+ int k;
+
+ if (!jw->pretty)
+ return;
+
+ for (k = 0; k < jw->open_stack.len; k++)
+ strbuf_addstr(>json, " ");
+}
+
+/*
+ * Begin an object or array (either top-level or nested within the currently
+ * open object or array).
+ */
+static void begin(struct json_writer *jw, char ch_open, int pretty)
+{
+ jw->pretty = pretty;
+
+ strbuf_addch(>json, ch_open);
+
+ strbuf_addch(>open_stack, ch_open);
+ jw->need_comma = 0;
+}
+
+/*
+ * Assert that the top of the open-stack is an object.
+ */
+static void assert_in_object(const struct json_writer *jw, const char *key)
+{
+ if (!jw->open_stack.len)
+ BUG("json-writer: object: missing jw_object_begin(): '%s'",
key);
+ if (jw->open_stack.buf[jw->open_stack.len - 1] != '{')
+ BUG("json-writer: object: not in object: '%s'", key);
+}
+
+/*
+ * Assert that the top of the open-stack is an array.
+ */
+static void assert_in_array(const struct json_writer *jw)
+{
+ if (!jw->open_stack.len)
+ BUG("json-writer: array: missing jw_array_begin()");
+ if
[PATCH v9] json_writer V9
From: Jeff Hostetler Here is V9 of my json-writer patches. Please replace the existing V5..V8 versions with this one. This version has been rebased onto v2.18.0-rc1 rather than 2.17 because of changes to the test-tool setup. I've incorporated all of the suggestions on the V8 version, including Eric's suggestion to make the test tool read test data from stdin rather than using command line arguments. I also incorporated Eric's PERLJSON lazy prereq suggestion and squashed the perl unit test into the main commit. Jeff Hostetler (1): json_writer: new routines to create JSON data Makefile| 2 + json-writer.c | 414 json-writer.h | 105 + t/helper/test-json-writer.c | 564 t/helper/test-tool.c| 1 + t/helper/test-tool.h| 1 + t/t0019-json-writer.sh | 331 ++ t/t0019/parse_json.perl | 52 8 files changed, 1470 insertions(+) create mode 100644 json-writer.c create mode 100644 json-writer.h create mode 100644 t/helper/test-json-writer.c create mode 100755 t/t0019-json-writer.sh create mode 100644 t/t0019/parse_json.perl -- 2.9.3
Re: `git merge --abort` does not run `git rerere clear`
Sam Kuper writes: > `man git-rerere` says: > >> clear >> >> Reset the metadata used by rerere if a merge resolution is to be >> aborted. Calling git am [--skip|--abort] or git rebase >> [--skip|--abort] will automatically invoke this command. > > It makes sense that `git am [--skip|--abort]` and `git rebase > [--skip|--abort]` would run `git rerere clear`. > > However, if they run it, then shouldn't `git merge --abort` run it, too? > > If not, then what is the reason why not, and might it be helpful for > this reason to be mentioned at some appropriate place in the > documentation? I do not think there was any reason, other than that those who added "git merge --abort" weren't as careful as they should have been ;-) The command is a mere synonym to "git reset --merge"; I am not so confident that "git reset --merge" should also clear the current rerere state. If (and this is a big if) "git reset --merge" should, probably the right place to do so would be remove_branch_state(), before the function removes merge_rr file. Doing so might allow us to lose calls to rerere_clear() individual codepaths of various "abort" implementations make, but that would certainly require careful thinking to avoid unintended regressions.
How to delete files and directories from git commit history?
Hi all, I have git 2.17.1 running on Void Linux 64 bit running the Linux 4.16.9_1 kernel, not available to the public in any way (yet). I have a repository in my project's working directory, and push to a bare repository on the hard disk. My project (call it myproject) had a directory (call it docs/propdir) that was unnecessary for the project, and I've decided I don't want to offer the files in that directory as free software. So I need to delete docs/propdir from all commits in the repository. I did the following, while in my working repository's myproject directory: git filter-branch --tree-filter 'rm -rf docs/propdir' HEAD After that command, I could git clone the working repo and then git checkout to a commit stage that used to have the directory and files, and they're not there. So far, so good. But then I view all filenames from that directory that have ever been in the project, as follows: git cat-file --buffer --batch-all-objects \ --batch-check='%(objecttype) %(objectname)' \ | grep ^c | cut -d " " -f 2 \ | xargs -n 1 git ls-tree -r | sort | uniq \ | grep propdir The preceding command lists directory docs/propdir and all the files it's ever contained. This makes me uneasy because if the filenames are still there, I wonder if there's a route to get the files with a git command. Second, I'd prefer that when my repo is exposed to the public, people not know this directory and these files ever existed. What command do I do to remove all mention of doc/propdir and its files from my git history? Thanks, SteveT Steve Litt June 2018 featured book: Twenty Eight Tales of Troubleshooting http://www.troubleshooters.com/28
`git merge --abort` does not run `git rerere clear`
`man git-rerere` says: > clear > > Reset the metadata used by rerere if a merge resolution is to be > aborted. Calling git am [--skip|--abort] or git rebase > [--skip|--abort] will automatically invoke this command. It makes sense that `git am [--skip|--abort]` and `git rebase [--skip|--abort]` would run `git rerere clear`. However, if they run it, then shouldn't `git merge --abort` run it, too? If not, then what is the reason why not, and might it be helpful for this reason to be mentioned at some appropriate place in the documentation? Thanks :)
Re: GDPR compliance best practices?
On Tuesday, June 12, 2018 09:12:19 PM Peter Backes wrote: > So? If a thousand lawyers claim 1+1=3, it becomes a > mathematical truth? No, but probably a legal "truth". :) -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 19/20] abbrev: support relative abbrev values
Ævar Arnfjörð Bjarmason writes:
> Change the core.abbrev config variable and the corresponding --abbrev
> command-line option to support relative values such as +1 or -1.
>
> Before Linus's e6c587c733 ("abbrev: auto size the default
> abbreviation", 2016-09-30) git would default to abbreviating object
> names to 7-hexdigits, and only picking longer SHA-1s as needed if that
> was ambiguous.
>
> That change instead set the default length as a function of the
> estimated current count of objects:
>
> Based on the expectation that we would see collision in a
> repository with 2^(2N) objects when using object names shortened
> to first N bits, use sufficient number of hexdigits to cover the
> number of objects in the repository. Each hexdigit (4-bits) we
> add to the shortened name allows us to have four times (2-bits) as
> many objects in the repository.
>
> By supporting relative values for core.abbrev we can allow users to
> consistently opt-in for either a higher or lower probability of
> collision, without needing to hardcode a given numeric value like
> "10", which would be overkill on some repositories, and far to small
> on others.
Nicely explained and calculated ;-)
> test_expect_success 'describe core.abbrev=[-+]1 and --abbrev=[-+]1' '
> - test_must_fail git -c core.abbrev=+1 describe | sed_g_tr_d_n >describe
> &&
> - test_must_fail git -c core.abbrev=-1 describe | sed_g_tr_d_n >describe
> &&
> + git -c core.abbrev=-1 describe | sed_g_tr_d_n >describe &&
> + test_byte_count = 6 describe &&
> +
> + git -c core.abbrev=+1 describe | sed_g_tr_d_n >describe &&
> + test_byte_count = 8 describe &&
Even though I see the point of supporting absurdly small absolute
values like 4, I do not quite see the point of supporting negative
relative values here. What's the expected use case?
> git log --abbrev=+1 --pretty=format:%h -1 | tr_d_n >log &&
> - test_byte_count = 4 log &&
> + test_byte_count = 8 log &&
This, together with many many others in the rest of the patch, is
cute but confusing in that the diff shows change from 4 to 8 due to
the redefinition of what abbrev=+1 means. I have a feeling that it
may not be worth doing it "right", but if we were doing it "right",
we would probably have done it in multiple steps:
- the earlier patches in this series that demonstrates
--abbrev=+1 is --abbrev=1 and core.abbrev=+1 is an error.
- ensure --abbrev=+1 is rejected as syntax error just like
core.abbrev=+1 was, without introducing relative values
- introduce relative value.
That way, the last step (which corresponds to this patch) would show
change from "log --abbrev=+1" failing due to syntax error to showing
abbreviated value that is slightly longer than the default.
But a I said, it may not be worth doing so. "Is it worth supporting
negative relative length?" still stands, though.
Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all
On Tue, Jun 12, 2018 at 05:48:49AM -0400, Jeff King wrote: > On Mon, Jun 11, 2018 at 09:43:02AM +, Kirill Smelkov wrote: > > > > Looking deeper, we do not need these trees and blobs at all. The problem > > > is really just a tag that peels to an object that is not otherwise a ref > > > tip, regardless of its type. > > > > Thanks for feedback and for coming up with the fix. Sure, I'm ok with > > moving the test into your patch. However, even if a test becomes > > different - narrowing down root of _current_ problem, I suggest to also > > keep explicitly testing tag-to-blob and tag-to-tree (and if we really > > also want tag-to-commit and tag-to-tag) behaviour. Reason is: if we skip > > those now, they can potentially break in the future. > > Yeah, I have no problem testing these cases separately. There's no bug > with them now, but it is a slightly uncommon case. My suggestion would > be to submit a patch that goes on top of mine that covers these cases. Ok, I will try to do it. > > I would also suggest to fix upload-pack, as it is just not consistent to > > reject sending objects that were advertised, and so can strike again > > some way in the future. After all git.git's fetch-pack is not the only > > git client that should be possible to interact with git.git's > > upload-pack on remote side, right? > > No, it's not the only client. At the same time, I am on the fence over > whether upload-pack's behavior is wrong or not. It depends what you take > a peeled advertisement line to mean. Does it mean: this object has been > advertised and clients should be able to fetch it? Or does it mean: by > the way, you may be interested to know the peeled value of this tag in > case you want to do tag-following? > > So far I think it has only meant the latter. I could see an argument for > the former, but any client depending on that would never have worked, > AFAICT. We could _make_ it work, but how would a client know which > server version it's talking to (and therefore whether it is safe to make > the request?). I think you'd have to add a capability to negotiate. I see. I don't know the details of the exchange, just it was surprising for outside observer that fetching what was advertised is rejected. For the reference there is no strong need for me for this to work anymore (please see below). > > I'm not sure, but I would say that `fetch-pack --all` from an empty > > repository should not fail and should just give empty output as fetch > > does. > > Yeah, that seems reasonable to me. The die() that catches this dates > back to 2005-era, and we later taught the "fetch" porcelain to handle > this. I don't _think_ anybody would be upset that the plumbing learned > to treat this as a noop. It's probably a one-liner change in > fetch_pack() to return early instead of dying. Ok, I will try to send related testcase, and it is indeed easy to find - the fix itself. > > For the reference all the cases presented here are real - they appear in > > our repositories on lab.nexedi.com for which I maintain the backup, and > > I've noticed them in the process of switching git-backup from using > > fetch to fetch-pack here: > > > > https://lab.nexedi.com/kirr/git-backup/blob/0ab7bbb6/git-backup.go#L436 > > I applaud you using the porcelain for your scripts, but I suspect that > fetch-pack by itself is not at all well-used or well-tested these days > (certainly this --all bug has been around for almost 6 years and is not > very hard to trigger in practice). I see; thanks for the warning. > If an extra connection isn't a problem, you might be better off with > "git ls-remote", and then picking through the results for refs of > interest, and then "git fetch-pack" to actually get the pack. That's how > git-fetch worked when it was a shell script (e.g., see c3a200120d, the > last shell version). Yes, this is what I ended up doing: https://lab.nexedi.com/kirr/git-backup/commit/899103bf but for another reason - to avoid repeating for every fetched repository slow (in case of my "big" destination backup repository) quickfetch() checking in every spawned `git fetch`: git-backup can build index of objects we already have ourselves only once at startup, and then in fetch, after checking lsremote output, consult that index, and if we see we already have everything for an advertised reference - just avoid giving it to fetch-pack to process. It turns out for many pulled repositories there is usually no references changed at all and this way fetch-pack can be skipped completely: https://lab.nexedi.com/kirr/git-backup/commit/3efed898 > It may also be sane to just use "git fetch", which I'd say is _fairly_ > safe to script. Of course I have no problem if you want to fix all of > the corner cases in fetch-pack. Just giving you fair warning. :) Thanks again for the warning. I'm happy the switch to fetch plumbing happenned on my side, and so far it is working well. Like I said above I cannot use `git fetch` as is,
Re: GDPR compliance best practices?
On Tue, Jun 12, 2018 at 11:56:13AM -0700, David Lang wrote: > [quoting github] > > It's important to remember that the Right to Erasure only applies to > personal data, not all data. It only applies to data a controller (GitHub, > for example) is processing _solely_ on the basis of consent. This is very obviously wrong. See Art. 17 GDPR. Consent is only one of the explicitly mentioned grounds for deletion (it is (1) lit b, but there's also a and c to f). > And it only > applies when there's not another legal reason to keep the data -- for > instance, if the data is no longer necessary for the purpose for which it > was collected. This incorrect claim is completely inverting the logic of Art. 17. The logic is clarly that if ANY of lit (a) to (f) is satisfied, the data must be deleted. It is not necessary for ALL of them to be satisfied. In particular, if the data is no longer necessary for the purpose for which it was collected, then THAT ALONE is grounds for erasure ((1) lit. a). It does not matter at all whether processing was consent-based or whether such consent was withdrawn. > We do not process Git commit history on the basis of consent. We have a > legitimate business purpose for collecting Git commit history: to maintain > the integrity of the Git commit record. It remains necessary for its purpose > for as long as a commit needs to be attributable to its committer. Right, but this merely justifies storing the data, not publishing it, or keeping it published, as I already explained at length. > At GitHub, as part of our Privacy By Design work, we offer ways for users to > set their own Git commit email data, so if an individual wants to remain > anonymous or pseudonymous, he or she can do so. Not only is this contradicting fundamentally what they just said in the previous sentence, it is not a justification for ignoring the right to erasure either. It is exactly the purpose of the right to erasure to get the data erased *after* the fact. > We also explain, in our > [Privacy > Statement](https://help.github.com/articles/github-privacy-statement), that > we are not able to delete personal data from the Git commit history once it > has been recorded. Privacy Statements are not a justification under GDPR for processing data or ignoring the right to erasure. And oh yes they are able. Rewriting history is a possibility, though an inconvenient one. I have pointed towards more convenient solutions. > I'll point out that not only did the Github lawyers need to sign off on this > stance, but the Microsoft lawyers would have looked at it as well as part of > their purchase of Github. So? If a thousand lawyers claim 1+1=3, it becomes a mathematical truth? Best wishes Peter -- Peter Backes, r...@helen.plasma.xg8.de
Re: [PATCH 20/20] abbrev: add a core.validateAbbrev setting
Martin Ågren writes: >> +This is especially useful in combination with the >> +`core.validateAbbrev` setting, or to get more future-proof hashes to >> +reference in the future in a repository whose number of objects is >> +expected to grow. > > Maybe s/validateAbbrev/validateAbbrev = false/? Perhaps, but even with =true it would equally be useful, as the point of this setting is to future-proofing. >> ++ >> +When printing abbreviated object names Git needs to look through the >> +local object store. This is an `O(log N)` operation assuming all the >> +objects are in a single pack file, but `X * O(log N)` given `X` pack >> +files, which can get expensive on some larger repositories. > > This might be very close to too much information. Not very close, but just too much information without crucial detail (i.e. log N times what constant???). I'd drop it. >> ++ >> +This setting changes that to `O(1)`, but with the trade-off that >> +depending on the value of `core.abbrev` we may be printing abbreviated >> +hashes that collide. It may not be technically wrong to say "This changes it to O(1)", but I think to most people it is more understandable to say "This changes it to zero" ;-) Setting this variable to false makes Git not to validate the abbreviation it produces uniquely identifies an object among the current set of objects in the repository. Depending on the value of `core.abbrev`, we may be printing abbreviated hashes that collide. Note that setting this variable to true (or leaving it unset) does not guarantee that an abbreviated hash will never collide with future objects in the repository (you need to set core.abbrevLength to a larger value for that). would be sufficient to clarify, and also nuke the following overly-detailed paragraph. >> ... Too see how likely this is, try running: >> ++ >> +--- >> +git log --all --pretty=format:%h --abbrev=4 | perl -nE 'chomp; say length' >> | sort | uniq -c | sort -nr >> +--- >> ++ >> +This shows how many commits were found at each abbreviation length. On >> +linux.git in June 2018 this shows a bit more than 750,000 commits, >> +with just 4 needing 11 characters to be fully abbreviated, and the >> +default heuristic picks a length of 12. > > These last few paragraphs seem like too much to me. Yeah, it goes to too low level a detail, especially with the "try running" part. I'd remove everything but "On linux.git in June ..." if I were writing it from the above. >> ++ >> +Even without `core.validateAbbrev=false` the results abbreviation >> +already a bit of a probability game. They're guaranteed at the moment >> +of generation, but as more objects are added, ambiguities may be >> +introduced. Likewise, what's unambiguous for you may not be for >> +somebody else you're communicating with, if they have their own clone. > > This seems more useful. Yes, but still overly verbose; I think rolling it in the single paragraph description like I showed above would be sufficient. >> ++ >> +Therefore the default of `core.validateAbbrev=true` may not save you >> +in practice if you're sharing the SHA-1 or noting it now to use after >> +a `git fetch`. You may be better off setting `core.abbrev` to >> +e.g. `+2` to add 2 extra characters to the SHA-1, and possibly combine >> +that with `core.validateAbbrev=false` to get a reasonable trade-off >> +between safety and performance. > > Makes sense. As before, I'd suggest s/SHA-1/object ID/. Likewise. If we were to keep it, then s/object ID/object name/.
Re: GDPR compliance best practices?
Adding one more datapoint here, I reached out to Github to find out their stance. Here is what I got back Quote: Thanks for reaching out to us about this. It's important to remember that the Right to Erasure only applies to personal data, not all data. It only applies to data a controller (GitHub, for example) is processing _solely_ on the basis of consent. And it only applies when there's not another legal reason to keep the data — for instance, if the data is no longer necessary for the purpose for which it was collected. We do not process Git commit history on the basis of consent. We have a legitimate business purpose for collecting Git commit history: to maintain the integrity of the Git commit record. It remains necessary for its purpose for as long as a commit needs to be attributable to its committer. At GitHub, as part of our Privacy By Design work, we offer ways for users to set their own Git commit email data, so if an individual wants to remain anonymous or pseudonymous, he or she can do so. We also explain, in our [Privacy Statement](https://help.github.com/articles/github-privacy-statement), that we are not able to delete personal data from the Git commit history once it has been recorded. End Quote I'll point out that not only did the Github lawyers need to sign off on this stance, but the Microsoft lawyers would have looked at it as well as part of their purchase of Github. David Lang
Re: [PATCH 04/20] abbrev tests: add tests for core.abbrev and --abbrev
Ævar Arnfjörð Bjarmason writes:
> diff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh
> new file mode 100755
> index 00..1c60f5ff93
> --- /dev/null
> +++ b/t/t0014-abbrev.sh
> @@ -0,0 +1,118 @@
> +#!/bin/sh
> +
> +test_description='test core.abbrev and related features'
> +
> +. ./test-lib.sh
> +
> +tr_d_n() {
> + tr -d '\n'
> +}
- I personally would prefer to see the reason for having a helper
function like this to be "make it easier to reason about", rather
"make it shorter to type". tr_d_n feels more about the latter; if
we aimed for the former, this would be called strip_LF or
something.
- In the existing stcipts, it seems that we prefer to spell tr args
with 3-octal, e.g. \012 for LF, \015 for CR.
- Also let's have SP on both sides of (), i.e.
funcname () {
for shell function definitions.
> +cut_tr_d_n_field_n() {
> + cut -d " " -f $1 | tr_d_n
> +}
Likewise. Name this not after how it does what it does, but after
what it does and why it does so. In other words, if your answer to
the question: "what does the caller want?" is "it wants to pick the
nth field", then name it "pick_nth_field" or something?
> +test_expect_success 'abbrev empty value handling differs ' '
> + test_must_fail git -c core.abbrev= log -1 --pretty=format:%h 2>stderr &&
> + test_i18ngrep "bad numeric config value.*invalid unit" stderr &&
> +
> + git branch -v --abbrev= | cut_tr_d_n_field_n 3 >branch &&
> + test_byte_count = 40 branch &&
Sounds like a good thing to unify. If anything, --options=value
should be stricter than vari.able=value but it is the other way
around.
> + git log --abbrev= -1 --pretty=format:%h >log &&
> + test_byte_count = 4 log &&
Makes readers wonder if 4 is about 3 hex plus terminating LF. The
reason why this works is because --pretty=format:%h (not --format=%h
or --pretty=tformat:%h) uses delimiter semantics and we won't need
any LF to show a single record.
If we use the helper to measure the length of hexadecimal digits, it
may make sense to add a wrapper around test_byte_count that strips
LF; that way, the caller can use --format=%h instead and there will
be one less thing for the reader to worry about.
> + git diff --raw --abbrev= HEAD~ >diff &&
> + cut_tr_d_n_field_n 3 diff.3 &&
> + test_byte_count = 4 diff.3 &&
> + cut_tr_d_n_field_n 4 diff.4 &&
> + test_byte_count = 4 diff.4 &&
These all depend on the fact that we do not have excessive number of
irrelevant objects to force us to abbreviate using more hexdigits
than the minimum 4, right? I _think_ that is a reasonable
assumption we can depend on, even across the hash function
transition. We may want to leave in-code comment, though.
> + test_must_fail git diff --raw --abbrev= --no-index X Y >diff &&
> + cut_tr_d_n_field_n 3 diff.3 &&
> + test_byte_count = 4 diff.3 &&
Re: [PATCH] builtin/send-pack: populate the default configs
On Tue, Jun 12, 2018 at 2:16 PM, Jonathan Nieder wrote: > Masaya Suzuki wrote: >> builtin/send-pack didn't call git_default_config, and because of this >> git push --signed didn't respect the username and email in gitconfig in >> the HTTP transport. >> >> Signed-off-by: Masaya Suzuki >> --- > send-pack is not a command served by a daemon so this is less > potentially scary than the corresponding potential change to > upload-pack or receive-pack. Some configuration this brings in: > > - core.askpass: allows specifying an arbitrary command to use to > ask for a password. Respecting this setting should be very useful, > even if it would be scary in a daemon. > > - core.pager: allows specifying an arbitrary command to use as a > pager, if pagination is on (but it shouldn't be on). > - core.logallrefupdates: whether to create reflogs for new refs > (including new remote-tracking refs). Good. > - core.abbrev: what length of abbreviations to use when printing > abbreviated object ids (good). > - core.compression, core.packedgitwindowsize, etc: pack generation > tunables (good). > - advice.*: would allow us to make "git push" produce configurable > advice (good!) > - etc This summary probably ought to be in the commit message itself (plus additional commentary implied by the ending "etc."). It's exactly the sort of information someone looking at this patch in the future will want to know to feel assured that such behavior changes were taken into account by the patch author.
Re: [PATCH] builtin/send-pack: report gpg config errors
On Tue, Jun 12, 2018 at 11:08 AM Jonathan Nieder wrote:
>
> Hi,
>
> Stefan Beller wrote:
>
> > Any caller except of git_gpg_config() except the one in send_pack_config()
> > handles the return value of git_gpg_config(). Also handle the return value
> > there.
> >
> > Signed-off-by: Stefan Beller
> > ---
> > builtin/send-pack.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
>
> The other callers do
except builtin/log, which I followed. :/
>
> int status;
>
> if (!strcmp(k, "key.i.handle")) {
> ...
> return ...;
> }
> return git_gpg_config(k, v, NULL);
This conflicts with Masayas patch, that has a return of the
standard options.
>
> or
>
> int status;
>
> if (!strcmp(k, "key.i.handle")) {
> ...
> return ...;
> }
>
> status = git_gpg_config(k, v, NULL);
> if (status)
> return status;
> ...
This looks like a good idea.
Will send a follow up patch.
Re: [PATCH] builtin/send-pack: populate the default configs
Hi, Masaya Suzuki wrote: > builtin/send-pack didn't call git_default_config, and because of this > git push --signed didn't respect the username and email in gitconfig in > the HTTP transport. > > Signed-off-by: Masaya Suzuki > --- > builtin/send-pack.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Do you have a set of commands I can run to reproduce this? Bonus points if they're in the form of a patch to t/, but commands I can manually run would be fine, too. > --- a/builtin/send-pack.c > +++ b/builtin/send-pack.c > @@ -121,7 +121,7 @@ static int send_pack_config(const char *k, const char *v, > void *cb) > } > } > } > - return 0; > + return git_default_config(k, v, cb); send-pack is not a command served by a daemon so this is less potentially scary than the corresponding potential change to upload-pack or receive-pack. Some configuration this brings in: - core.askpass: allows specifying an arbitrary command to use to ask for a password. Respecting this setting should be very useful, even if it would be scary in a daemon. - core.pager: allows specifying an arbitrary command to use as a pager, if pagination is on (but it shouldn't be on). - core.logallrefupdates: whether to create reflogs for new refs (including new remote-tracking refs). Good. - core.abbrev: what length of abbreviations to use when printing abbreviated object ids (good). - core.compression, core.packedgitwindowsize, etc: pack generation tunables (good). - advice.*: would allow us to make "git push" produce configurable advice (good!) - etc So it looks like a very good change. Thanks for writing it. Reviewed-by: Jonathan Nieder
Re: [PATCH 03/20] blame doc: explicitly note how --abbrev=40 gives 39 chars
Ævar Arnfjörð Bjarmason writes: > In a later change I'm adding stress testing of the commit abbreviation > as it relates to git-blame and others, and initially thought that the > inability to extract full SHA-1s from the non-"--porcelain" output was > a bug. ... meaning that it is not actually a bug, as the output format other than porcelain is for human consumption? > --- a/Documentation/git-blame.txt > +++ b/Documentation/git-blame.txt > @@ -88,6 +88,11 @@ include::blame-options.txt[] > Instead of using the default 7+1 hexadecimal digits as the > abbreviated object name, use +1 digits. Note that 1 column > is used for a caret to mark the boundary commit. This is outside the scope of this patch, but is the above 7+1 still current or do we need updating it for the (not so) recent change to auto-scale the default abbreviation width? > ++ > +Because of this UI design, the only way to get the full SHA-1 of the > +boundary commit is to use the `--porcelain` format. With `--abbrev=40` > +only 39 characters of the boundary SHA-1 will be emitted, since one > +will be used for the caret to mark the boundary. OK.
Re: [PATCH] builtin/send-pack: report gpg config errors
Hi,
Stefan Beller wrote:
> Any caller except of git_gpg_config() except the one in send_pack_config()
> handles the return value of git_gpg_config(). Also handle the return value
> there.
>
> Signed-off-by: Stefan Beller
> ---
> builtin/send-pack.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
The other callers do
int status;
if (!strcmp(k, "key.i.handle")) {
...
return ...;
}
return git_gpg_config(k, v, NULL);
or
int status;
if (!strcmp(k, "key.i.handle")) {
...
return ...;
}
status = git_gpg_config(k, v, NULL);
if (status)
return status;
...
Should this do the same? That way, if config_fn_t learns to return
more fine-grained errors (e.g. a new return value of -2 or 1) then it
would be propagated.
Thanks,
Jonathan
Re: [PATCH] builtin/send-pack: report gpg config errors
On Tue, Jun 12, 2018 at 1:53 PM, Stefan Beller wrote: > Any caller except of git_gpg_config() except the one in send_pack_config() Drop the first "except"? Also: s/Any caller/All callers/ > handles the return value of git_gpg_config(). Also handle the return value > there. > > Signed-off-by: Stefan Beller
[PATCH] builtin/send-pack: report gpg config errors
Any caller except of git_gpg_config() except the one in send_pack_config()
handles the return value of git_gpg_config(). Also handle the return value
there.
Signed-off-by: Stefan Beller
---
> Makes sense. I wonder if this was deliberate
There was no need for other configuration to be loaded and signing
pushes is the first that needs repository configuration.
Looking at 68c757f2199 (push: add a config option push.gpgSign for default
signed pushes, 2015-08-19) further, maybe we want to also have this?
Thanks,
Stefan
builtin/send-pack.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 4923b1058c6..b54a0cae878 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -101,7 +101,8 @@ static void print_helper_status(struct ref *ref)
static int send_pack_config(const char *k, const char *v, void *cb)
{
- git_gpg_config(k, v, NULL);
+ if (git_gpg_config(k, v, NULL) < 0)
+ return -1;
if (!strcmp(k, "push.gpgsign")) {
const char *value;
--
2.18.0.rc1.244.gcf134e6275-goog
Re: [PATCH] list-objects: check if filter is NULL before using
Jonathan Tan writes:
> In partial_clone_get_default_filter_spec(), the
> core_partial_clone_filter_default variable may be NULL; ensure that it
> is not NULL before using it.
>
> Signed-off-by: Jonathan Tan
> ---
> This was noticed by someone else at $DAY_JOB when trying to use a
> partial clone with no core.partialclonefilter set.
Thanks, will queue.
> ---
> list-objects-filter-options.c | 2 ++
> t/t0410-partial-clone.sh | 8
> 2 files changed, 10 insertions(+)
>
> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> index 6a3cc985c4..c0e2bd6a06 100644
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c
> @@ -146,6 +146,8 @@ void partial_clone_get_default_filter_spec(
> /*
>* Parse default value, but silently ignore it if it is invalid.
>*/
> + if (!core_partial_clone_filter_default)
> + return;
> gently_parse_list_objects_filter(filter_options,
>core_partial_clone_filter_default,
>NULL);
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index cc18b75c03..4984ca583d 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -23,7 +23,15 @@ promise_and_delete () {
> delete_object repo "$HASH"
> }
>
> +test_expect_success 'extensions.partialclone without filter' '
> + test_create_repo server &&
> + git clone --filter="blob:none" "file://$(pwd)/server" client &&
> + git -C client config --unset core.partialclonefilter &&
> + git -C client fetch origin
> +'
> +
> test_expect_success 'missing reflog object, but promised by a commit, passes
> fsck' '
> + rm -rf repo &&
> test_create_repo repo &&
> test_commit -C repo my_commit &&
Re: [PATCH] builtin/send-pack: populate the default configs
Masaya Suzuki writes: > builtin/send-pack didn't call git_default_config, and because of this > git push --signed didn't respect the username and email in gitconfig in > the HTTP transport. > > Signed-off-by: Masaya Suzuki > --- > builtin/send-pack.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Makes sense. I wonder if this was deliberate (i.e. we had some reason not wanting to be affected by some _other_ configuration items and refraining from doing the default-config was the way to implement it), but I do not think of a reason offhand. Will queue. Thanks. > > diff --git a/builtin/send-pack.c b/builtin/send-pack.c > index 4923b1058..42fd8d1a3 100644 > --- a/builtin/send-pack.c > +++ b/builtin/send-pack.c > @@ -121,7 +121,7 @@ static int send_pack_config(const char *k, const char *v, > void *cb) > } > } > } > - return 0; > + return git_default_config(k, v, cb); > } > > int cmd_send_pack(int argc, const char **argv, const char *prefix)
Re: BUG: submodule code prints '(null)'
On Sat, Jun 9, 2018 at 4:04 AM Duy Nguyen wrote: > > On Tue, Jun 05, 2018 at 05:31:41PM +0200, Duy Nguyen wrote: > > I do not know how to reproduce this (and didn't bother to look deeply > > into it after I found it was not a trivial fix) but one of my "git > > fetch" showed > > > > warning: Submodule in commit be2db96a6c506464525f588da59cade0cedddb5e > > at path: '(null)' collides with a submodule named the same. Skipping > > it. > > The problem is default_name_or_path() can return NULL when a submodule > is not populated. The fix could simply be printing path instead of > name (because we are talking about path in the commit message), like > below. > > But I don't really understand c68f837576 (implement fetching of moved > submodules - 2017-10-16), the commit that made this change, and not > sure if we should be reporting name here or path. Heiko? That change is quite interesting as I did not understand it at first sight as well. See https://public-inbox.org/git/20171016135827.gc12...@book.hvoigt.net/ and the follow ups, specifically https://public-inbox.org/git/20171019181109.27792-2-sbel...@google.com/ that tries to clean up the code, but was ultimately dropped.
[PATCH] builtin/send-pack: populate the default configs
builtin/send-pack didn't call git_default_config, and because of this git push --signed didn't respect the username and email in gitconfig in the HTTP transport. Signed-off-by: Masaya Suzuki --- builtin/send-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 4923b1058..42fd8d1a3 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -121,7 +121,7 @@ static int send_pack_config(const char *k, const char *v, void *cb) } } } - return 0; + return git_default_config(k, v, cb); } int cmd_send_pack(int argc, const char **argv, const char *prefix) -- 2.18.0.rc1.242.g61856ae69a-goog
Re: [PATCHv2 0/6] git-p4: some small fixes updated
Luke Diamand writes:
> This is an updated version of the set of changes I posted recently,
> following comments on the list:
>
> disable automatic sync after git-p4 submit:
> https://marc.info/?l=git=152818734814838=2
>
> better handling of being logged out by Perforce:
>https://marc.info/?l=git=152818893815326=2
>
> adapt the block size automatically on git-p4 submit:
>https://marc.info/?l=git=152819004315688=2
>
> - Spelling corrections (Eric)
> - Improved comments (Eric)
> - Exception class hierarchy fix (Merland)
> - test simplification (Eric)
>
That reminds me of one thing.
This 6-patch series depends on the rm/p4-submit-with-commit-option
that came without and still waiting for a sign-off by the original
author. Also I do not think the original patch reached the public
list, so I'm attaching the patch to make sure people know which
patch I am talking about.
Romain, can we get your sign-off on the patch you sent earlier?
Thanks.
-- >8 --
From: Romain Merland
Date: Wed, 9 May 2018 17:32:12 +0200
Subject: [PATCH] git-p4: add options --commit and --disable-rebase
On a daily work with multiple local git branches, the usual way to
submit only a specified commit was to cherry-pick the commit on
master then run git-p4 submit. It can be very annoying to switch
between local branches and master, only to submit one commit. The
proposed new way is to select directly the commit you want to
submit.
Add option --commit to command 'git-p4 submit' in order to submit
only specified commit(s) in p4.
On a daily work developping software with big compilation time, one
may not want to rebase on his local git tree, in order to avoid long
recompilation.
Add option --disable-rebase to command 'git-p4 submit' in order to
disable rebase after submission.
Reviewed-by: Luke Diamand
Signed-off-by: Junio C Hamano
---
Documentation/git-p4.txt | 14 ++
git-p4.py| 29 +++--
t/t9807-git-p4-submit.sh | 40
3 files changed, 77 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index d8c8f11c9f..88d109debb 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -149,6 +149,12 @@ To specify a branch other than the current one, use:
$ git p4 submit topicbranch
+To specify a single commit or a range of commits, use:
+
+$ git p4 submit --commit
+$ git p4 submit --commit
+
+
The upstream reference is generally 'refs/remotes/p4/master', but can
be overridden using the `--origin=` command-line option.
@@ -330,6 +336,14 @@ These options can be used to modify 'git p4 submit'
behavior.
p4/master. See the "Sync options" section above for more
information.
+--commit |::
+Submit only the specified commit or range of commits, instead of the full
+list of changes that are in the current Git branch.
+
+--disable-rebase::
+Disable the automatic rebase after all commits have been successfully
+submitted.
+
Rebase options
~~
These options can be used to modify 'git p4 rebase' behavior.
diff --git a/git-p4.py b/git-p4.py
index 7bb9cadc69..f4a6f3b4c3 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1352,7 +1352,12 @@ def __init__(self):
optparse.make_option("--update-shelve", dest="update_shelve",
action="append", type="int",
metavar="CHANGELIST",
help="update an existing shelved
changelist, implies --shelve, "
- "repeat in-order for multiple
shelved changelists")
+ "repeat in-order for multiple
shelved changelists"),
+optparse.make_option("--commit", dest="commit",
metavar="COMMIT",
+ help="submit only the specified
commit(s), one commit or xxx..xxx"),
+optparse.make_option("--disable-rebase",
dest="disable_rebase", action="store_true",
+ help="Disable rebase after submit is
completed. Can be useful if you "
+ "work from a local git branch that is not
master")
]
self.description = "Submit changes from git to the perforce depot."
self.usage += " [name of git branch to submit into perforce depot]"
@@ -1362,6 +1367,8 @@ def __init__(self):
self.dry_run = False
self.shelve = False
self.update_shelve = list()
+self.commit = ""
+self.disable_rebase = False
self.prepare_p4_only = False
self.conflict_behavior = None
self.isWindows = (platform.system() == "Windows")
@@ -2103,9 +2110,18 @@ def run(self, args):
else:
commitish = 'HEAD'
-for line in read_pipe_lines(["git", "rev-list", "--no-merges",
"%s..%s" % (self.origin, commitish)]):
-
Re: [PATCH v6 12/21] commit-graph: verify parent list
Derrick Stolee writes: > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh > index 2b9214bc83..9a3481c30f 100755 > --- a/t/t5318-commit-graph.sh > +++ b/t/t5318-commit-graph.sh > @@ -269,6 +269,9 @@ GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET + > $HASH_LEN * 8)) > GRAPH_BYTE_OID_LOOKUP_MISSING=$(($GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN * 4 + > 10)) > GRAPH_COMMIT_DATA_OFFSET=$(($GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN * > $NUM_COMMITS)) > GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSET > +GRAPH_BYTE_COMMIT_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN)) > +GRAPH_BYTE_COMMIT_EXTRA_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 4)) > +GRAPH_BYTE_COMMIT_WRONG_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 3)) There seems to depend on having GRAPH_COMMIT_DATA_OFFSET in this file, but I do not think 'next' or sb/object-store-alloc has one, and I do not think I saw an earlier patch in this series to add such a line. It appears that many messages in this series are sent in quoted-printable when they do not have to, which made me say fuzzy things like "I do not think I saw" in the previous paragraph, instead of being able to be definitive with a simple "grep" result. Please try to see if you can find a way to tell your MUA not to do unnecessary mail munging, if you can do so easily. The worst one was 07/21, which came in base64 and contained a patch to this file in MS-DOS line ending convention, which made it impossible to apply. > > # usage: corrupt_graph_and_verify > # Manipulates the commit-graph file at the position > @@ -348,4 +351,19 @@ test_expect_success 'detect incorrect tree OID' ' > "root tree OID for commit" > ' > > +test_expect_success 'detect incorrect parent int-id' ' > + corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_PARENT "\01" \ > + "invalid parent" > +' > + > +test_expect_success 'detect extra parent int-id' ' > + corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_EXTRA_PARENT "\00" \ > + "is too long" > +' > + > +test_expect_success 'detect wrong parent' ' > + corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_WRONG_PARENT "\01" \ > + "commit-graph parent for" > +' > + > test_done
Re: [ANNOUNCE] Git v2.16.0-rc0
Jeff King writes: > To be honest, I could easily see an argument that I _should_ be setting > GIT_SSH_VARIANT to explain what my wrapper is expecting, even though it > happened to work before. The way I read that message is that the patch proposed in https://public-inbox.org/git/20180103050730.ga87...@aiede.mtv.corp.google.com is "lesser of two evils" in that it is still evil because somebody still has to be asked to explicitly set "variant" anyway but the changing what 'simple' means in the middle would mean those who did not have to set it now have to. So, you should be setting it, even if we adopt the patch, right? ;-) > But it seems like this discussion ended in > favor of calling this a regression that should be fixed, and AFAICT > nothing happened after. So I thought I'd ping and mention one more data > point. My impression is that regression "fix" does not exist---rather, it was a proposal (and it may make a better tradeoff than the status quo) to help users of older OpenSSH by inconveniencing users of different implementations that do -4/6/p differently from OpenSSH. In any case, from where I sit, I am still waiting for this offer by Jonathan > It's good you caught this flaw in the detection. Would something like > the following make sense? If so, I can resend with a commit message > and tests tomorrow or the day after. to be followed up ;-)
Re: Hash algorithm analysis
Hi, On 10/06/18 00:49, brian m. carlson wrote: > I imported the optimized 64-bit implementation of KangarooTwelve. The > AVX2 implementation was not considered for licensing reasons (it's > partially generated from external code, which falls foul of the GPL's > "preferred form for modifications" rule). Indeed part of the AVX2 code in the Keccak code package is an extension of the implementation in OpenSSL (written by Andy Polyakov). The assembly code is generated by a Perl script, and we extended it to fit in the KCP's internal API. Would it solve this licensing problem if we remap our extensions to the Perl script, which would then become "the source"? On 12/06/18 00:35, brian m. carlson wrote: >> My understanding is that all the algorithms we're discussing are >> believed to be approximately equivalent in security. That's a strange >> thing to say when e.g. K12 uses fewer rounds than SHA3 of the same >> permutation, but it is my understanding nonetheless. We don't know >> yet how these hash algorithms will ultimately break. > > With the exception of variations in preimage security, I expect that's > correct. I think implementation availability and performance are the > best candidates for consideration. Note that we recently updated the paper on K12 (accepted at ACNS 2018), with more details on performance and security. https://eprint.iacr.org/2016/770 >> My understanding of the discussion so far: >> >> Keccak team encourages us[1] to consider a variant like K12 instead >> of SHA3. > > While I think K12 is an interesting algorithm, I'm not sure we're > going to get as good of performance out of it as we might want due to > the lack of implementations. Implementation availability is indeed important. The effort to transform an implementation of SHAKE128 into one of K12 is limited due to the reuse of their main components (round function, sponge construction). So the availability of SHA-3/Keccak implementations can benefit that of K12 if there is sufficient interest. E.g., the SHA-3/Keccak instructions in ARMv8.2 can speed up K12 as well. Kind regards, Gilles
Re: [GSoC][PATCH 1/1] rebase--interactive: rewrite the edit-todo functionality in C
Hi Alban On 12/06/18 13:33, Alban Gruin wrote: Hi Phillip, Le 11/06/2018 à 17:32, Phillip Wood a écrit : + if (launch_editor(todo_file, NULL, NULL)) I'm not sure that this will respect GIT_SEQUENCE_EDITOR, it would be nice to have a launch_sequence_editor() function that shared as much code as possible with launch_editor() It could be done by making launch_editor() and launch_sequence_editor() some kind of wrapper around a function like launch_specified_editor() (or something like that), that would take the editor as a parameter, in addition to the path, the buffer and environment variables. It would be also very trivial to implement your first point above on top of that. That sounds like a good way forward, launch_sequence_editor() could just call launch_editor() if GIT_SEQUENCE_EDITOR and sequence.editor are not set. int append_todo_help(unsigned edit_todo, unsigned keep_empty); Can this declaration be removed now? No, it’s still used in rebase--helper.c for now. Ah, sorry I missed that Best Wishes Phillip +int edit_todo_list(unsigned flags); int skip_unnecessary_picks(void); int rearrange_squash(void); Best Wishes Phillip Cheers, Alban
Re: [GSoC][PATCH 1/1] rebase--interactive: rewrite the edit-todo functionality in C
Hi Elijah, Le 12/06/2018 à 17:46, Elijah Newren a écrit : > Hi Alban, > > On Mon, Jun 11, 2018 at 6:57 AM, Alban Gruin wrote: >> This rewrites the edit-todo functionality from shell to C. >> >> To achieve that, a new command mode, `edit-todo`, is added, and the >> `write-edit-todo` flag is removed, as the shell script does not need to >> write the edit todo help message to the todo list anymore. >> >> The shell version is then stripped in favour of a call to the helper. > > I looked over the patch and didn't see any problems (though I haven't > worked with rebase--helper before, or the code for todo list editing), > but when I went to apply the patch it failed telling me: > > Applying: rebase--interactive: rewrite the edit-todo functionality in C > error: sha1 information is lacking or useless (builtin/rebase--helper.c). > error: could not build fake ancestor > Patch failed at 0001 rebase--interactive: rewrite the edit-todo > functionality in C > Use 'git am --show-current-patch' to see the failed patch > > I tried each of master, next, and pu (as of today) to see if it might > apply there. On which commit is this patch based? (Do you have other > local commits that this was based on top of?) > > > Elijah > It can be applied on top of pu with my patch that rewrites append_todo_help() in C (https://public-inbox.org/git/20180607103012.22981-1-alban.gr...@gmail.com/) Cheers, Alban
Re: [RFC PATCH v1] telemetry design overview (part 1)
Jeff King writes: > In my experience the maintenance burden is not in the "connect to a > socket" part, but the fact that you have to sprinkle the entry points > throughout the code (e.g., "set 'cloning' flag to 1" or "I'm entering > the pack generation phase of the fetch"). So I'd love to see us do that > sprinkling _once_ where everyone can benefit, whether they want better > tracing for debugging, telemetry across their installed base, or > whatever. I tend to agree. Transport (or file) can stay outside the core of this "telemetry" thing---agreeing on what and when to trace, and how the trace is represented, and having an API and solid guideline, would allow us to annotate the code just once and get useful data in a consistent way. > The mechanism to handle those calls is much easier to plug in and out, > then. And I don't have a huge preference for compile-time versus > run-time, or whether bits are in-tree or out-of-tree. Obviously we'd > have some basic "write to stderr or a file" consumer in-tree. If it makes readers fearful of big brother feel safer, I think we probably can add code that is runtime controllable that is compiled in conditionally ;-) I do not quite buy "Big brothers who want this for their own in house use case can afford to patch" argument, by the way. If anything, having a solid tracing mechanism (to which existing GIT_TRACE support _should_ be able to serve as a starting point) would mean small budget guys do not have to do their own investing to collect useful data over their customer base (I am seeing an analog in "popcon", opt-in stats of installed package in a distro, which we can view as "telemetry data for distro installer program").
Re: [GSoC][PATCH 1/1] rebase--interactive: rewrite the edit-todo functionality in C
Hi Alban, On Mon, Jun 11, 2018 at 6:57 AM, Alban Gruin wrote: > This rewrites the edit-todo functionality from shell to C. > > To achieve that, a new command mode, `edit-todo`, is added, and the > `write-edit-todo` flag is removed, as the shell script does not need to > write the edit todo help message to the todo list anymore. > > The shell version is then stripped in favour of a call to the helper. I looked over the patch and didn't see any problems (though I haven't worked with rebase--helper before, or the code for todo list editing), but when I went to apply the patch it failed telling me: Applying: rebase--interactive: rewrite the edit-todo functionality in C error: sha1 information is lacking or useless (builtin/rebase--helper.c). error: could not build fake ancestor Patch failed at 0001 rebase--interactive: rewrite the edit-todo functionality in C Use 'git am --show-current-patch' to see the failed patch I tried each of master, next, and pu (as of today) to see if it might apply there. On which commit is this patch based? (Do you have other local commits that this was based on top of?) Elijah
Re: [PATCH 05/23] midx: write header information to lockfile
On Thu, Jun 7, 2018 at 7:01 PM Derrick Stolee wrote:
> diff --git a/midx.c b/midx.c
> index 616af66b13..3e55422a21 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -1,9 +1,62 @@
> #include "git-compat-util.h"
> #include "cache.h"
> #include "dir.h"
> +#include "csum-file.h"
> +#include "lockfile.h"
> #include "midx.h"
>
> +#define MIDX_SIGNATURE 0x4d494458 /* "MIDX" */
> +#define MIDX_VERSION 1
> +#define MIDX_HASH_VERSION 1 /* SHA-1 */
...
> +static size_t write_midx_header(struct hashfile *f,
> + unsigned char num_chunks,
> + uint32_t num_packs)
> +{
> + char byte_values[4];
> + hashwrite_be32(f, MIDX_SIGNATURE);
> + byte_values[0] = MIDX_VERSION;
> + byte_values[1] = MIDX_HASH_VERSION;
Quoting from "State of NewHash work, future directions, and discussion" [1]
* If you need to serialize an algorithm identifier into your data
format, use the format_id field of struct git_hash_algo. It's
designed specifically for that purpose.
[1]
https://public-inbox.org/git/20180612024252.ga141...@aiede.svl.corp.google.com/T/#m5fdd09dcaf31266c45343fb6c0beaaa3e928bc60
> + byte_values[2] = num_chunks;
> + byte_values[3] = 0; /* unused */
> + hashwrite(f, byte_values, sizeof(byte_values));
> + hashwrite_be32(f, num_packs);
> +
> + return MIDX_HEADER_SIZE;
> +}
--
Duy
Re: [GSoC][PATCH 1/1] rebase--interactive: rewrite the edit-todo functionality in C
Hi Phillip, Le 11/06/2018 à 17:32, Phillip Wood a écrit : >> + if (launch_editor(todo_file, NULL, NULL)) > > I'm not sure that this will respect GIT_SEQUENCE_EDITOR, it would be > nice to have a launch_sequence_editor() function that shared as much > code as possible with launch_editor() > It could be done by making launch_editor() and launch_sequence_editor() some kind of wrapper around a function like launch_specified_editor() (or something like that), that would take the editor as a parameter, in addition to the path, the buffer and environment variables. It would be also very trivial to implement your first point above on top of that. >> int append_todo_help(unsigned edit_todo, unsigned keep_empty); > > Can this declaration be removed now? No, it’s still used in rebase--helper.c for now. > >> +int edit_todo_list(unsigned flags); >> int skip_unnecessary_picks(void); >> int rearrange_squash(void); > > Best Wishes > > Phillip > Cheers, Alban
Re: [BUG] git-rebase: reword squashes commits in case of merge-conflicts
On Mon, Jun 11, 2018 at 06:06:11PM +0200, ch wrote:
> After the rebase the 'stuff' branch only has a single commit even though I'd
> expect there to be two according to the instructions that were passed to
> git-rebase. It works as expected if there's either no merge-conflict at the
> reword or if the conflicting commit is applied as 'pick'.
>
> I'm running git version 2.17.1.windows.2. I also tried native git version
> 2.7.4
> via WSL (running Ubuntu 16.04.4 LTS) and this version does not exhibit this
> behavior.
Thanks for a thorough report. I couldn't reproduce it on v2.17.1 on
Linux, which makes me wonder if the issue is related to git-for-windows
somehow. To the best of my knowledge (and a quick scan of "git diff"
results) the code should be the same, though.
I'm cc-ing Johannes, who is both the resident interactive-rebase expert
_and_ the Git for Windows maintainer. Copious quoting below.
-Peff
-- >8 --
> Hi all!
>
> During a recent rebase operation on one of my repositories a number of commits
> unexpectedly ended up getting squashed into other commits. After some
> experiments it turned out that the 'reword' instruction seems to squash the
> referenced commit into the preceding commit if there's a merge-conflict.
>
> Here's a small reproduction recipe to illustrate the behavior:
>
>1. Create a small test repository using the following Bash script:
>
>
> function add_file()
> {
> echo "$1" > "$2"
> git add "$2"
> git commit -m "$3"
> }
>
> git init .
>
> add_file "test" "file-1" "master"
>
> git checkout -b stuff
>
> add_file "aaa" "file-2" "feature_a"
> add_file "bbb" "file-2" "fixup! feature_a"
> add_file "ccc" "file-2" "fixup! feature_a"
>
> add_file "ddd" "file-2" "feature_b"
> add_file "eee" "file-2" "fixup! feature_b"
> add_file "fff" "file-2" "fixup! feature_b"
>
>
>2. Run
>
> $ git rebase --autosquash --interactive --onto master master stuff
>
> to interactively rebase 'stuff' onto 'master'. This should generate the
> following todo-stream:
>
>
> pick ... feature_a
> fixup ... fixup! feature_a
> fixup fixup! feature_a
> pick feature_b
> fixup ... fixup! feature_b
> fixup ... fixup! feature_b
>
>
>3. Remove the fixup line right before the second pick (i.e. 'fixup
> ')
> in order to enforce a merge-conflict later when applying commit
> .
>
>4. Replace the second pick instruction (i.e. 'pick ') with a
> reword.
>
>5. Launch the rebase operation. It should fail at the 'reword '
> instruction due to a merge-conflict.
>
>6. Resolve the conflict by taking the remote-side (i.e. 'ddd'), add the
> change to the index with git-add and run
>
> $ git rebase --continue
>
> This should spawn the commit message editor and after saving and
> quitting
> the rebase should finish cleanly.
>
> After the rebase the 'stuff' branch only has a single commit even though I'd
> expect there to be two according to the instructions that were passed to
> git-rebase. It works as expected if there's either no merge-conflict at the
> reword or if the conflicting commit is applied as 'pick'.
>
> I'm running git version 2.17.1.windows.2. I also tried native git version
> 2.7.4
> via WSL (running Ubuntu 16.04.4 LTS) and this version does not exhibit this
> behavior.
>
> - ch
Re: [PATCH] git-rebase.sh: handle keep-empty like all other options
On 11/06/18 17:08, Elijah Newren wrote: > Hi Phillip, > > On Mon, Jun 11, 2018 at 8:16 AM, Phillip Wood > wrote: >> On 11/06/18 15:42, Elijah Newren wrote: > >>> However, we have a bigger problem with empty commits, in that there >>> are really three modes rather than two: >>>* Automatically drop empty commits (default for am-based rebase) >>>* Automatically keep empty commits (as done with --keep-empty) >>>* Halt the rebase and tell the user how to specify if they want to >>> keep it (default for interactive rebases) >>> >>> Currently, only the first option is available to am-based rebases, and >>> only the second two options are available to interactive-based >>> rebases. >> >> >> I'm not sure that's the case, my understanding is that for an interactive >> rebase unless you give '--keep-empty' then any empty commits will be >> commented out in the todo list and therefore dropped unless they're >> uncommented when editing the list. > > Ah, yes, you are right; I was implicitly thinking about cases where > the commit became empty rather than when the commit started > empty...and mis-read --keep-empty (which as I learned now is only for > started-empty cases). > >> The third case happens when a commit >> becomes empty when it's rebased (i.e. the original commit is not empty), I'm >> not sure what the am backend does for this. > > The am backend does not halt the rebase; it simply drops the commit > and says "No changes -- Patch already applied." > > It has always seemed jarring and confusing to me that one rebase mode > stops and asks the user what to do and the other assumes. I think > both should have the same default (and have options to pick the > alternate behavior). > > I'm less certain what the default should be, though. I'm not really sure either, on the one hand most of the time it is convenient for rebase just to skip over it, on the other if you have some important information in the commit message or a note then maybe you want rebase to stop so you can selvage that information. > >> cherry-pick has >> '--keep-redundant-commits' for this case but that has never been added to >> rebase. On path forwards is to always stop, and implement --keep-redundant-commits for rebase. That would be very easy for interactive rebases as it shares the same code as cherry-pick. I've just had a quick look at builtin/am.c and I think it would be fairly straightforward to add some code to check if it's rebasing and stop if the patch is already applied. Best Wishes Phillip > Thanks for the pointer. >
Re: [PATCH] fsck: avoid looking at NULL blob->object
On Sat, Jun 09, 2018 at 03:44:30PM +0200, Martin Ågren wrote: > On 9 June 2018 at 11:21, Jeff King wrote: > > On Sat, Jun 09, 2018 at 10:50:36AM +0200, Martin Ågren wrote: > > > >> On 9 June 2018 at 10:32, Jeff King wrote: > >> > Except it _does_ do one non-trivial thing, which is call the > >> > report() function, which wants us to pass a pointer to a > >> > "struct object". Which we don't have (we have only a "struct > >> > object_id"). So we erroneously passed the NULL object, which > >> > >> s/passed/dereferenced/? Probably doesn't affect the fix though. > > > > Well, we passed it, and then that function dereferenced it. :) > > > > I'm going to re-roll for the minor bits that Eric pointed out, so I'll > > try to word this better. > > My bad. I somehow thought we get into trouble already before we call > `report()`. Well, we do, since we have undefined behavior. But for all > practical purposes `>object` and `blob` are the same > (NULL-)pointer so we only crash after we call `report()`. > > Anyway, obviously no need to do anything about this in a v3. Ah, yeah, I didn't really think of it that way. But certainly you are right that the moment we look at >object, we are invoking undefined behavior according to the standard. Hopefully the wording tweak I made covers both ways of thinking about it. :) -Peff
Re: [PATCH] completion: correct zsh detection when run from git-completion.zsh (Re: [PATCH v2] completion: reduce overhead of clearing cached --options)
On 11 June 2018 at 20:20, Jonathan Nieder wrote:
> SZEDER Gábor wrote:
>
>> Being in RC phase, I'm all for aiming for a minimal solution.
>> However, I don't think that the better fix would be erm.. any "less
>> minimal":
>
> Thanks again. May we have your sign-off?
>
> contrib/completion/git-completion.bash | 5 -
> contrib/completion/git-completion.zsh | 2 +-
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index 12814e9bbf..f4a2e6774b 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -3223,7 +3223,10 @@ __gitk_main ()
> __git_complete_revlist
> }
>
> -if [[ -n ${ZSH_VERSION-} ]]; then
> +if [[ -n ${ZSH_VERSION-} ]] &&
> + # Don't define these functions when sourced from 'git-completion.zsh',
> + # it has its own implementations.
> + [[ -z ${GIT_SOURCING_ZSH_COMPLETION-} ]]; then
> echo "WARNING: this script is deprecated, please see
> git-completion.zsh" 1>&2
>
> autoload -U +X compinit && compinit
> diff --git a/contrib/completion/git-completion.zsh
> b/contrib/completion/git-completion.zsh
> index 53cb0f934f..049d6b80f6 100644
> --- a/contrib/completion/git-completion.zsh
> +++ b/contrib/completion/git-completion.zsh
> @@ -39,7 +39,7 @@ if [ -z "$script" ]; then
> test -f $e && script="$e" && break
> done
> fi
> -ZSH_VERSION='' . "$script"
> +GIT_SOURCING_ZSH_COMPLETION=y . "$script"
>
> __gitcomp ()
> {
> --
> 2.18.0.rc1.242.g61856ae69a
The change looks good to me :)
Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all
On Mon, Jun 11, 2018 at 09:43:02AM +, Kirill Smelkov wrote: > > Looking deeper, we do not need these trees and blobs at all. The problem > > is really just a tag that peels to an object that is not otherwise a ref > > tip, regardless of its type. > > Thanks for feedback and for coming up with the fix. Sure, I'm ok with > moving the test into your patch. However, even if a test becomes > different - narrowing down root of _current_ problem, I suggest to also > keep explicitly testing tag-to-blob and tag-to-tree (and if we really > also want tag-to-commit and tag-to-tag) behaviour. Reason is: if we skip > those now, they can potentially break in the future. Yeah, I have no problem testing these cases separately. There's no bug with them now, but it is a slightly uncommon case. My suggestion would be to submit a patch that goes on top of mine that covers these cases. > I would also suggest to fix upload-pack, as it is just not consistent to > reject sending objects that were advertised, and so can strike again > some way in the future. After all git.git's fetch-pack is not the only > git client that should be possible to interact with git.git's > upload-pack on remote side, right? No, it's not the only client. At the same time, I am on the fence over whether upload-pack's behavior is wrong or not. It depends what you take a peeled advertisement line to mean. Does it mean: this object has been advertised and clients should be able to fetch it? Or does it mean: by the way, you may be interested to know the peeled value of this tag in case you want to do tag-following? So far I think it has only meant the latter. I could see an argument for the former, but any client depending on that would never have worked, AFAICT. We could _make_ it work, but how would a client know which server version it's talking to (and therefore whether it is safe to make the request?). I think you'd have to add a capability to negotiate. > I'm not sure, but I would say that `fetch-pack --all` from an empty > repository should not fail and should just give empty output as fetch > does. Yeah, that seems reasonable to me. The die() that catches this dates back to 2005-era, and we later taught the "fetch" porcelain to handle this. I don't _think_ anybody would be upset that the plumbing learned to treat this as a noop. It's probably a one-liner change in fetch_pack() to return early instead of dying. > For the reference all the cases presented here are real - they appear in > our repositories on lab.nexedi.com for which I maintain the backup, and > I've noticed them in the process of switching git-backup from using > fetch to fetch-pack here: > > https://lab.nexedi.com/kirr/git-backup/blob/0ab7bbb6/git-backup.go#L436 I applaud you using the porcelain for your scripts, but I suspect that fetch-pack by itself is not at all well-used or well-tested these days (certainly this --all bug has been around for almost 6 years and is not very hard to trigger in practice). If an extra connection isn't a problem, you might be better off with "git ls-remote", and then picking through the results for refs of interest, and then "git fetch-pack" to actually get the pack. That's how git-fetch worked when it was a shell script (e.g., see c3a200120d, the last shell version). It may also be sane to just use "git fetch", which I'd say is _fairly_ safe to script. Of course I have no problem if you want to fix all of the corner cases in fetch-pack. Just giving you fair warning. :) -Peff
Re: [PATCH] completion: correct zsh detection when run from git-completion.zsh (Re: [PATCH v2] completion: reduce overhead of clearing cached --options)
On Mon, Jun 11, 2018 at 8:20 PM, Jonathan Nieder wrote:
> From: SZEDER Gábor
> Subject: completion: correct zsh detection when run from git-completion.zsh
>
> v2.18.0-rc0~90^2 (completion: reduce overhead of clearing cached
> --options, 2018-04-18) worked around a bug in bash's "set" builtin on
> MacOS by using compgen instead. It was careful to avoid breaking zsh
> by guarding this workaround with
>
> if [[ -n ${ZSH_VERSION-}} ]]
>
> Alas, this interacts poorly with git-completion.zsh's bash emulation:
>
> ZSH_VERSION='' . "$script"
>
> Correct it by instead using a new GIT_SOURCING_ZSH_COMPLETION shell
> variable to detect whether git-completion.bash is being sourced from
> git-completion.zsh. This way, the zsh variant is used both when run
> from zsh directly and when run via git-completion.zsh.
>
> Reproduction recipe:
>
> 1. cd git/contrib/completion && cp git-completion.zsh _git
> 2. Put the following in a new ~/.zshrc file:
>
> autoload -U compinit; compinit
> autoload -U bashcompinit; bashcompinit
> fpath=(~/src/git/contrib/completion $fpath)
>
> 3. Open zsh and "git ".
>
> With this patch:
> Triggers nice git-completion.bash based tab completion
>
> Without:
> contrib/completion/git-completion.bash:354: read-only variable: QISUFFIX
> zsh:12: command not found: ___main
> zsh:15: _default: function definition file not found
> _dispatch:70: bad math expression: operand expected at `/usr/bin/g...'
> Segmentation fault
Btw, even if ZSH were not to segfault, the resulting shell would be
next to useless. The thing is that 'compgen -v foo' is supposed to
list only variable names starting with 'foo', at least that's what it
does under Bash. In ZSH's Bash emulation, however, it lists _all_
variable names, including such fundamental env vars like PATH, HOME,
and IFS. So when ZSH took the wrong if branch and run that
unset $(compgen -v __gitcomp_builtin_)
then it would unset PATH and everything else as well.
Re: [PATCH 2/2] git-rebase: error out when incompatible options passed
On 11/06/18 16:49, Elijah Newren wrote: > Another thing I missed... > > On Sun, Jun 10, 2018 at 12:40 PM, Phillip Wood > wrote: >> On 07/06/18 06:06, Elijah Newren wrote: > >>> Some exceptions I left out: >>> >>>* --merge and --interactive are technically incompatible since they are >>> supposed to run different underlying scripts, but with a few small >>> changes, --interactive can do everything that --merge can. In fact, >>> I'll shortly be sending another patch to remove git-rebase--merge and >>> reimplement it on top of git-rebase--interactive. >> >> Excellent I've wondered about doing that but never got round to it. One >> thing I was slightly concerned about was that someone maybe parsing the >> output of git-rebase--merge and they'll get a nasty shock when that output >> changes as a result of using the sequencer. > > I can see the minor worry, but I think upon inspection it's not > something that concerns me, for a few reasons: > > In terms of use, given that rebase --merge was introduced to handle > renames in mid-2006, but plain rebase has been able to handle them > since late 2008 (though directory renames changes that again), the > utility of rebase --merge has been somewhat limited. I think that > limits the exposure. But to address the 'break' more directly... > > If we were to agree that we needed to support folks parsing rebase > output, that would be a really strict requirement that I think would > prevent lots of fixes. And if so, it's one we've violated a number of > times. For example, I certainly wasn't thinking about rebase when I > modified messages in merge-recursive.c over the years, but they'd leak > through for rebase --merge. (Those messages would not leak through to > rebase --interactive as much, since the sequencer sets o.buffer_output > and then only conditionally shows the output.) Also, changes that > have occurred in the past, like adding 'git gc --auto' to rebase, > modifying error messages directly found in git-rebase--merge.sh would > have been considered breaks. > > Finally, looking over all the differences in output while fixing up > testcases makes me think we've done much less around designing the > output based on what we want the user to see, and more around what > minimal fixups can we do to these lower level commands that provide > useful functionality to the user? We linearize history differently > for different rebase modes, have different entries in the reflog > depending on which mode, and we often times implement features for > just one mode and then sometimes add it to others. In fact, I think > the primary reason that am-based and merge-based rebases had a --quiet > option and the interactive rebases didn't, is mostly attributable to > the defaults of the lower level commands these three were built on top > of (git-am vs. git-merge-recursive vs. git-cherry-pick). The noiser > two merited a quiet option, and the option was never added for the > last one. > > Anyway, that's my rationale. I'm curious to hear if folks disagree or > see things I'm overlooking or have reasons I might be weighting > tradeoffs less than optimally. > I agree that there are already plenty of inconsistencies, (it's great to see you reducing them). If we can avoid emulating the ouput of git-rebase--merge.sh in sequencer.c that would definitely be my preferred option (the code is already a bit hard to follow in places where there it's doing slightly different things for cherry-pick and rebase -i). Hopefully no one is relying on the output, as you say it's just whatever the underlying plumbing prints rather than designed for a specific purpose. Best Wishes Phillip
Re: "linking" to files from another repo
On Tue, Jun 12, 2018 at 10:18:31AM +0200, Crni Gorac wrote: > I'm working on a large closed-source project. For one of clients, I > had to create a library that consists of some directories from > original project, and even within these directories, not all files are > used for the library. On top of that, I've added some files specific > for this library, in separate directories and in the library repo > top-level directory. Most of files from original project are > unchanged, and for some, I had to make some small changes (mostly to > exclude dependencies on other stuff from the original project). We're > now switching to Git from another VCS, and I'm wondering is there any > way to automatically "link" those pieces from the main project repo > into my library repo? So far, I would run update in the main project > repo, check is any of files that I'm using in library changed, and if > so, then I would either copy the new version of given file into the > library (if given file unchanged for the library), or merge updates > manually (if given file is one of these files that are slightly > changed for the library), and then commit all these changes in the > library repo. None of changes in the library will ever go back into > the main project, i.e. the flow of updates is uni-directional here. > So, any support in Git to automate the procedure of updating the > library with the changes made for corresponding files in the main > project? If I understand your case correctly, this is usually solved with a "vendor" branch type of workflow. E.g.: # Import the first version of the library verbatim onto the branch # "upstream". git init git checkout -b upstream tar xf the-lib-1.0.tar git add . git commit -m 'import version 1.0' # Now make your changes on master. git checkout -b master upstream hack hack hack git commit -m 'fixed some bits' # Time passes. You want to import 1.1. Do this on the upstream branch, # where we know that we can overwrite the state completely (since we # are replacing upstream's 1.0 with their 1.1). git checkout upstream rm -rf * tar xf the-lib-1.1.tar git add -A git commit -m 'import version 1.1' # And now you can merge 1.1 into your master branch. The git history # you've created reflects two lines of development: upstream's and # your custom changes. So merging will never "undo" the changes you've # made (but you'll probably see conflicts if you deleted a file and # upstream changed it, for example). git checkout master git merge upstream Note that your "master" branch doesn't have to be _just_ the changes to upstream. It can actually be your whole project, complete with changes to the upstream library. -Peff
Re: [PATCH] checkout files in-place
On Tue, Jun 12, 2018 at 09:13:54AM +0300, Orgad Shaneh wrote: > Some of my colleagues use an ancient version of Source Insight, which also > locks files for write. If that application is locking files for writing (that is to say, it did not specify the `FILE_SHARE_WRITE` bit in the sharing modes during `CreateFile`) then this patch would not help. Applications, generally speaking, should be locking files for write. It's the default in Win32 and .NET's file open APIs because few applications are prepared to detect and support a file changing out from underneath them in the middle of a read. > It's less important than it was before those fixes, but it is still needed > for users of Qt Creator 4.6 (previous versions just avoided mmap, 4.7 uses > mmap only for system headers). Other tools on Windows might as well > misbehave. I don't understand what mmap'ing via `CreateFileMapping` has to do with this. It takes an existing `HANDLE` that was opened with `CreateFile`, which is where the sharing mode was supplied. I would be surprised if there are other tools on Windows that have specified `FILE_SHARE_WRITE` but not `FILE_SHARE_DELETE`. Generally speaking, if you don't care about another process changing a file underneath you then you should specify both. If you do then you should specify neither. I'm not saying that git shouldn't work around a bug in QT Creator - that's not my call, though I would be loathe to support this configuration option in libgit2. But I am saying that it seems like this patch doesn't have broad applicability beyond that particular tool. -ed
Re: [PATCH] completion: correct zsh detection when run from git-completion.zsh (Re: [PATCH v2] completion: reduce overhead of clearing cached --options)
On Mon, Jun 11, 2018 at 8:20 PM, Jonathan Nieder wrote:
> From: SZEDER Gábor
> Subject: completion: correct zsh detection when run from git-completion.zsh
>
> v2.18.0-rc0~90^2 (completion: reduce overhead of clearing cached
> --options, 2018-04-18) worked around a bug in bash's "set" builtin on
> MacOS by using compgen instead. It was careful to avoid breaking zsh
> by guarding this workaround with
>
> if [[ -n ${ZSH_VERSION-}} ]]
>
> Alas, this interacts poorly with git-completion.zsh's bash emulation:
>
> ZSH_VERSION='' . "$script"
>
> Correct it by instead using a new GIT_SOURCING_ZSH_COMPLETION shell
> variable to detect whether git-completion.bash is being sourced from
> git-completion.zsh. This way, the zsh variant is used both when run
> from zsh directly and when run via git-completion.zsh.
>
> Reproduction recipe:
>
> 1. cd git/contrib/completion && cp git-completion.zsh _git
> 2. Put the following in a new ~/.zshrc file:
>
> autoload -U compinit; compinit
> autoload -U bashcompinit; bashcompinit
> fpath=(~/src/git/contrib/completion $fpath)
>
> 3. Open zsh and "git ".
>
> With this patch:
> Triggers nice git-completion.bash based tab completion
>
> Without:
> contrib/completion/git-completion.bash:354: read-only variable: QISUFFIX
> zsh:12: command not found: ___main
> zsh:15: _default: function definition file not found
> _dispatch:70: bad math expression: operand expected at `/usr/bin/g...'
> Segmentation fault
>
> Reported-by: Rick van Hattem
> Reported-by: Dave Borowitz
> Signed-off-by: Jonathan Nieder
> ---
> SZEDER Gábor wrote:
>
>> Being in RC phase, I'm all for aiming for a minimal solution.
>> However, I don't think that the better fix would be erm.. any "less
>> minimal":
>
> Thanks again. May we have your sign-off?
Sure:
Signed-off-by: SZEDER Gábor
Thank you for the commit message; had a family visit the last couple
of days and could not get around to it.
> contrib/completion/git-completion.bash | 5 -
> contrib/completion/git-completion.zsh | 2 +-
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index 12814e9bbf..f4a2e6774b 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -3223,7 +3223,10 @@ __gitk_main ()
> __git_complete_revlist
> }
>
> -if [[ -n ${ZSH_VERSION-} ]]; then
> +if [[ -n ${ZSH_VERSION-} ]] &&
> + # Don't define these functions when sourced from 'git-completion.zsh',
> + # it has its own implementations.
> + [[ -z ${GIT_SOURCING_ZSH_COMPLETION-} ]]; then
> echo "WARNING: this script is deprecated, please see
> git-completion.zsh" 1>&2
>
> autoload -U +X compinit && compinit
> diff --git a/contrib/completion/git-completion.zsh
> b/contrib/completion/git-completion.zsh
> index 53cb0f934f..049d6b80f6 100644
> --- a/contrib/completion/git-completion.zsh
> +++ b/contrib/completion/git-completion.zsh
> @@ -39,7 +39,7 @@ if [ -z "$script" ]; then
> test -f $e && script="$e" && break
> done
> fi
> -ZSH_VERSION='' . "$script"
> +GIT_SOURCING_ZSH_COMPLETION=y . "$script"
>
> __gitcomp ()
> {
> --
> 2.18.0.rc1.242.g61856ae69a
>
"linking" to files from another repo
I'm working on a large closed-source project. For one of clients, I had to create a library that consists of some directories from original project, and even within these directories, not all files are used for the library. On top of that, I've added some files specific for this library, in separate directories and in the library repo top-level directory. Most of files from original project are unchanged, and for some, I had to make some small changes (mostly to exclude dependencies on other stuff from the original project). We're now switching to Git from another VCS, and I'm wondering is there any way to automatically "link" those pieces from the main project repo into my library repo? So far, I would run update in the main project repo, check is any of files that I'm using in library changed, and if so, then I would either copy the new version of given file into the library (if given file unchanged for the library), or merge updates manually (if given file is one of these files that are slightly changed for the library), and then commit all these changes in the library repo. None of changes in the library will ever go back into the main project, i.e. the flow of updates is uni-directional here. So, any support in Git to automate the procedure of updating the library with the changes made for corresponding files in the main project? Thanks.
Re: Guten Tag
Sie wurden ausgewählt, um eine Spende / Prämiensumme zu erhalten, kontaktieren Sie wbrde...@gmail.com für weitere Informationen. E-Mail: wbrde...@gmail.com
Re: [PATCH 01/10] t: add tool to translate hash-related values
On Mon, Jun 11, 2018 at 9:05 PM, brian m. carlson
wrote:
> On Mon, Jun 11, 2018 at 03:47:43AM -0400, Eric Sunshine wrote:
>> The word "translate" is very generic and is (at least in my mind)
>> strongly associated with i18n/l10n, so the name test_translate() may
>> be confusing for readers. Perhaps test_oid_lookup() or test_oid_get()
>> or even just test_oid()?
>
> test_oid would be fine. One note is that this doesn't always produce
> OIDs; sometimes it will produce other values, but as long as you don't
> think that's too confusing, I'm fine with it.
It was surprising to see it used for non-OID's (such as hash
characteristics), but not hard to deal with.
One could also view this as a generic key/value cache (not specific to
OID's) with overriding super-key (the hash algorithm, in this case),
which would allow for more generic name than test_oid(), but we don't
have to go there presently.
>> This is a very expensive lookup since it invokes a heavyweight command
>> (perl, in this case) for *every* OID it needs to retrieve from the
>> file. Windows users, especially, will likely not be happy about this.
>> See below for an alternative.
>
> I agree perl would be expensive if it were invoked frequently, but
> excepting SHA1-prerequisite tests, this function is invoked 32 times in
> the entire testsuite.
>
> One of the reasons I chose perl was because we have a variety of cases
> where we'll need spaces in values, and those tend to be complex in
> shell.
Can you give examples of cases in which values will contain spaces? It
wasn't obvious from this patch series that such a need would arise.
Are these values totally free-form? If not, some character (such as
"_", "-", ".", etc.) could act as a stand-in for space. That shouldn't
be too hard to handle.
>> Here's what I had envisioned when reading your emails about OID lookup
>> table functionality:
>>
>> --- >8 ---
>> test_oid_cache () {
>> while read tag rest
>> do
>> case $tag in \#*) continue ;; esac
>>
>> for x in $rest
>> do
>> k=${x%:*}
>> v=${x#*:}
>> if test "$k" = $test_hash_algo
>> then
>> eval "test_oid_$tag=$v"
>> break
>> fi
>> done
>> done
>> }
>
> Using shell variables like this does have the downside that we're
> restricted to only characters allowed in shell variables. That was
> something I was trying to avoid, but it certainly isn't fatal.
Is that just a general concern or do you have specific "weird" keys in mind?
>> test_detect_hash() would detect the hash algorithm and record it
>> instead of having to determine it each time an OID needs to be
>> "translated". It probably would be called by test-lib.sh.
>
> We'll probably have to deal with multiple hashes in the future,
> including for input and output, but this could probably be coerced to
> handle that case.
My original version of test_oid_cache() actually allowed for that by
caching _all_ information from the tables rather than only values
corresponding to $test_hash_algo. It looked like this:
--- >8 ---
test_oid_cache () {
while read tag rest
do
case $tag in \#*) continue ;; esac
for x in $rest
do
eval "test_oid_${tag}_${x%:*}=${x#*:}"
done
done
}
--- >8 ---
The hash algorithm is incorporated into the cache variable name like
this: "test_oid_hexsz_sha256"
>> And, when specifying values from which to choose based upon hash
>> algorithm:
>>
>> $(test_oid bored sha1:deadbeef NewHash:feedface)
>
> This syntax won't exactly be usable, because we have to deal with spaces
> in values. It shouldn't be too much of a problem to just use
> test_oid_cache at the top of the file, though.
See above about possibly using a stand-in character for space.
>> A nice property of how this implementation caches values is that you
>> don't need test_oid() for really simple cases. You can just access the
>> variable directly. For instance: $test_oid_hexsz
>
> Because we're going to need multiple hash support in the future, for
> input, output, and on-disk, I feel like this is not a good direction for
> us to go in the testsuite. Internally, those variable names are likely
> to change.
Indeed, this isn't a real selling point; I only just thought of it
while composing the mail. Going through the API is more robust.
(Although, see above how the revised test_oid_cache() incorporates the
hash algorithm into the cache variable name.)
Re: git-gui ignores core.hooksPath
Am 11.06.2018 um 23:58 schrieb Stefan Beller: On Mon, Jun 4, 2018 at 10:48 PM Bert Wesarg wrote: the last time this topic came up, Stefan (in Cc) offered to volunteer. Stefan, is this offer still open? I would support this. After I made this offer, I started looking at the code base more and trying to add a feature just to discover I do not qualify as fluent in tcl/tk. Also I have some issues managing my time, so I retract that offer. Though I'd still review the code in this area. Maybe it's of interest to somebody: My current pet fun project is this one: https://github.com/j6t/git-gui-ng -- rewrite to C++ using Tk as GUI toolkit. I use git gui (the original) daily, but no, I don't volunteer to keep it going. Tcl is just too exotic. -- Hannes
