* [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
From: Nguyễn Thái Ngọc Duy @ 2016-09-28 11:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20160928114348.1470-1-pclouds@gmail.com>
The original commit d95d728aba06a34394d15466045cbdabdada58a2 was
reverted in commit 78cc1a540ba127b13f2f3fd531777b57f3a9cd46 because we
were (and still are) not ready for a new world order. A lot more
investigation must be done to see what is impacted. See the 78cc1a5 for
details.
This patch takes a smaller and safer step. The new behavior is
controlled by shift_ita flag. We can gradually move more diff users to
the new behavior after we are sure it's safe to do so. This flag is
exposed to outside temporarily as "--shift-ita" for people who prefer
"git diff [--cached] --stat" to "git status"
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/diff-options.txt | 7 +++++++
diff-lib.c | 12 ++++++++++++
diff.c | 2 ++
diff.h | 1 +
t/t2203-add-intent.sh | 20 ++++++++++++++++++--
t/t7064-wtstatus-pv2.sh | 4 ++--
wt-status.c | 7 ++++++-
7 files changed, 48 insertions(+), 5 deletions(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 7805a0c..e63285c 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -575,5 +575,12 @@ endif::git-format-patch[]
--line-prefix=<prefix>::
Prepend an additional prefix to every line of output.
+--shift-ita::
+ By default entries added by "git add -N" appear as an existing
+ empty file in "git diff" and a new file in "git diff --cached".
+ This option makes the entry appear as a new file in "git diff"
+ and non-existent in "git diff --cached". Experimental option,
+ could be removed in future.
+
For more detailed explanation on these common options, see also
linkgit:gitdiffcore[7].
diff --git a/diff-lib.c b/diff-lib.c
index 3007c85..62d67c8 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -214,6 +214,11 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
!is_null_oid(&ce->oid),
ce->name, 0);
continue;
+ } else if (revs->diffopt.shift_ita && ce_intent_to_add(ce)) {
+ diff_addremove(&revs->diffopt, '+', ce->ce_mode,
+ EMPTY_BLOB_SHA1_BIN, 0,
+ ce->name, 0);
+ continue;
}
changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
@@ -379,6 +384,13 @@ static void do_oneway_diff(struct unpack_trees_options *o,
struct rev_info *revs = o->unpack_data;
int match_missing, cached;
+ /* i-t-a entries do not actually exist in the index */
+ if (revs->diffopt.shift_ita && idx && ce_intent_to_add(idx)) {
+ idx = NULL;
+ if (!tree)
+ return; /* nothing to diff.. */
+ }
+
/* if the entry is not checked out, don't examine work tree */
cached = o->index_only ||
(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));
diff --git a/diff.c b/diff.c
index c6da383..4178689 100644
--- a/diff.c
+++ b/diff.c
@@ -3923,6 +3923,8 @@ int diff_opt_parse(struct diff_options *options,
return parse_submodule_opt(options, arg);
else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
return parse_ws_error_highlight(options, arg);
+ else if (!strcmp(arg, "--shift-ita"))
+ options->shift_ita = 1;
/* misc options */
else if (!strcmp(arg, "-z"))
diff --git a/diff.h b/diff.h
index ec76a90..5dd4f9c 100644
--- a/diff.h
+++ b/diff.h
@@ -146,6 +146,7 @@ struct diff_options {
int dirstat_permille;
int setup;
int abbrev;
+ int shift_ita;
/* white-space error highlighting */
#define WSEH_NEW 1
#define WSEH_CONTEXT 2
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 8f22c43..c6a4648 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -5,10 +5,24 @@ test_description='Intent to add'
. ./test-lib.sh
test_expect_success 'intent to add' '
+ test_commit 1 &&
+ git rm 1.t &&
+ echo hello >1.t &&
echo hello >file &&
echo hello >elif &&
git add -N file &&
- git add elif
+ git add elif &&
+ git add -N 1.t
+'
+
+test_expect_success 'git status' '
+ git status --porcelain | grep -v actual >actual &&
+ cat >expect <<-\EOF &&
+ DA 1.t
+ A elif
+ A file
+ EOF
+ test_cmp expect actual
'
test_expect_success 'check result of "add -N"' '
@@ -43,7 +57,9 @@ test_expect_success 'i-t-a entry is simply ignored' '
git add -N nitfol &&
git commit -m second &&
test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
- test $(git diff --name-only HEAD -- nitfol | wc -l) = 1
+ test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
+ test $(git diff --name-only --shift-ita HEAD -- nitfol | wc -l) = 0 &&
+ test $(git diff --name-only --shift-ita -- nitfol | wc -l) = 1
'
test_expect_success 'can commit with an unrelated i-t-a entry in index' '
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index 3012a4d..e319fa2 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -246,8 +246,8 @@ test_expect_success 'verify --intent-to-add output' '
git add --intent-to-add intent1.add intent2.add &&
cat >expect <<-EOF &&
- 1 AM N... 000000 100644 100644 $_z40 $EMPTY_BLOB intent1.add
- 1 AM N... 000000 100644 100644 $_z40 $EMPTY_BLOB intent2.add
+ 1 .A N... 000000 000000 100644 $_z40 $_z40 intent1.add
+ 1 .A N... 000000 000000 100644 $_z40 $_z40 intent2.add
EOF
git status --porcelain=v2 >actual &&
diff --git a/wt-status.c b/wt-status.c
index 9a14658..5f9b1cd 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -437,7 +437,7 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
switch (p->status) {
case DIFF_STATUS_ADDED:
- die("BUG: worktree status add???");
+ d->mode_worktree = p->two->mode;
break;
case DIFF_STATUS_DELETED:
@@ -547,6 +547,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
setup_revisions(0, NULL, &rev, NULL);
rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
DIFF_OPT_SET(&rev.diffopt, DIRTY_SUBMODULES);
+ rev.diffopt.shift_ita = 1;
if (!s->show_untracked_files)
DIFF_OPT_SET(&rev.diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);
if (s->ignore_submodule_arg) {
@@ -570,6 +571,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
setup_revisions(0, NULL, &rev, &opt);
DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
+ rev.diffopt.shift_ita = 1;
if (s->ignore_submodule_arg) {
handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
} else {
@@ -605,6 +607,8 @@ static void wt_status_collect_changes_initial(struct wt_status *s)
if (!ce_path_match(ce, &s->pathspec, NULL))
continue;
+ if (ce_intent_to_add(ce))
+ continue;
it = string_list_insert(&s->change, ce->name);
d = it->util;
if (!d) {
@@ -911,6 +915,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
init_revisions(&rev, NULL);
DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
+ rev.diffopt.shift_ita = 1;
memset(&opt, 0, sizeof(opt));
opt.def = s->is_initial ? EMPTY_TREE_SHA1_HEX : s->reference;
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 2/3] diff-lib.c: enable --shift-ita in index_differs_from()
From: Nguyễn Thái Ngọc Duy @ 2016-09-28 11:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20160928114348.1470-1-pclouds@gmail.com>
This function is basically "git diff --cached HEAD", It has three
callers:
- One in builtin/commit.c, which uses it to determine if the index is
different from HEAD and go ahead making a new commit.
- Two in sequencer.c, which use it to see if the index is dirty.
In the first case, if ita entries are present, index_differs_from() may
report "dirty". However at tree creation phase, ita entries are dropped
and the result tree may look exactly the same as HEAD (assuming that
nothing else is changed in index). This is what we need index_differs_from()
for, to catch new empty commits. Enabling shift_ita in index_differs_from()
fixes this.
In the second case, the presence of ita entries are enough to say the
index is dirty and not continue on. Make an explicit check for that
before comparing index against HEAD (whether --shift-ita is present is
irrelevant)
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
cache.h | 1 +
diff-lib.c | 1 +
read-cache.c | 10 ++++++++++
sequencer.c | 5 +++--
t/t2203-add-intent.sh | 11 +++++++++++
5 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index d0494c8..1ddd515 100644
--- a/cache.h
+++ b/cache.h
@@ -561,6 +561,7 @@ extern int do_read_index(struct index_state *istate, const char *path,
extern int read_index_from(struct index_state *, const char *path);
extern int is_index_unborn(struct index_state *);
extern int read_index_unmerged(struct index_state *);
+extern int has_ita_entries(struct index_state *);
#define COMMIT_LOCK (1 << 0)
#define CLOSE_LOCK (1 << 1)
extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);
diff --git a/diff-lib.c b/diff-lib.c
index 62d67c8..ea55ee2 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -545,6 +545,7 @@ int index_differs_from(const char *def, int diff_flags)
DIFF_OPT_SET(&rev.diffopt, QUICK);
DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
rev.diffopt.flags |= diff_flags;
+ rev.diffopt.shift_ita = 1;
run_diff_index(&rev, 1);
if (rev.pending.alloc)
free(rev.pending.objects);
diff --git a/read-cache.c b/read-cache.c
index 31eddec..f6a5f61 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1674,6 +1674,16 @@ int is_index_unborn(struct index_state *istate)
return (!istate->cache_nr && !istate->timestamp.sec);
}
+int has_ita_entries(struct index_state *istate)
+{
+ int i;
+
+ for (i = 0; i < istate->cache_nr; i++)
+ if (ce_intent_to_add(istate->cache[i]))
+ return 1;
+ return 0;
+}
+
int discard_index(struct index_state *istate)
{
int i;
diff --git a/sequencer.c b/sequencer.c
index eec8a60..10cded0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -469,7 +469,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
unborn = get_sha1("HEAD", head);
if (unborn)
hashcpy(head, EMPTY_TREE_SHA1_BIN);
- if (index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : "HEAD", 0))
+ if (has_ita_entries(&the_index) ||
+ index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : "HEAD", 0))
return error_dirty_index(opts);
}
discard_cache();
@@ -1064,7 +1065,7 @@ static int sequencer_continue(struct replay_opts *opts)
if (ret)
return ret;
}
- if (index_differs_from("HEAD", 0))
+ if (has_ita_entries(&the_index) || index_differs_from("HEAD", 0))
return error_dirty_index(opts);
todo_list = todo_list->next;
return pick_commits(todo_list, opts);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index c6a4648..aa06415 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -129,5 +129,16 @@ test_expect_success 'cache-tree does skip dir that becomes empty' '
)
'
+test_expect_success 'commit: ita entries ignored in empty commit check' '
+ git init empty-subsequent-commit &&
+ (
+ cd empty-subsequent-commit &&
+ test_commit one &&
+ : >two &&
+ git add -N two &&
+ test_must_fail git commit -m nothing-new-here
+ )
+'
+
test_done
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 3/3] commit: don't be fooled by ita entries when creating initial commit
From: Nguyễn Thái Ngọc Duy @ 2016-09-28 11:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20160928114348.1470-1-pclouds@gmail.com>
ita entries are dropped at tree creation phase. If the entire index
consists of just ita entries, the result would be a a commit with no
entries, which should be caught unless --allow-empty is specified.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/commit.c | 11 ++++++++---
t/t2203-add-intent.sh | 10 ++++++++++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index bb9f79b..56b24cb 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -894,9 +894,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
if (amend)
parent = "HEAD^1";
- if (get_sha1(parent, sha1))
- commitable = !!active_nr;
- else {
+ if (get_sha1(parent, sha1)) {
+ int i, ita_nr = 0;
+
+ for (i = 0; i < active_nr; i++)
+ if (ce_intent_to_add(active_cache[i]))
+ ita_nr++;
+ commitable = active_nr - ita_nr > 0;
+ } else {
/*
* Unless the user did explicitly request a submodule
* ignore mode by passing a command line option we do
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index aa06415..65314fc 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -129,6 +129,16 @@ test_expect_success 'cache-tree does skip dir that becomes empty' '
)
'
+test_expect_success 'commit: ita entries ignored in empty intial commit check' '
+ git init empty-intial-commit &&
+ (
+ cd empty-intial-commit &&
+ : >one &&
+ git add -N one &&
+ test_must_fail git commit -m nothing-new-here
+ )
+'
+
test_expect_success 'commit: ita entries ignored in empty commit check' '
git init empty-subsequent-commit &&
(
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* Re: [PATCH 0/3] i-t-a entries in git-status, and git-commit
From: Duy Nguyen @ 2016-09-28 11:51 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20160928114348.1470-1-pclouds@gmail.com>
On Wed, Sep 28, 2016 at 6:43 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> 1) and 2) are fixed by changing the position of ita entries in diff
> code. ita entries should be seen as a new file when compared between
> worktree and HEAD
Big typo. "between worktree and index".
--
Duy
^ permalink raw reply
* Re: [PATCH v2 01/11] i18n: add--interactive: mark strings for translation
From: Vasco Almeida @ 2016-09-28 12:43 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
David Aguilar
In-Reply-To: <xmqqr387y4le.fsf@gitster.mtv.corp.google.com>
A Dom, 25-09-2016 às 15:52 -0700, Junio C Hamano escreveu:
> > @@ -252,7 +253,7 @@ sub list_untracked {
> > }
> >
> > my $status_fmt = '%12s %12s %s';
> > -my $status_head = sprintf($status_fmt, 'staged', 'unstaged', 'path');
> > +my $status_head = sprintf($status_fmt, __('staged'), __('unstaged'), __('path'));
>
> Wouldn't it make sense to allow translators to tweak $status_fmt if
> you are allowing the earlier elements that are formatted with %12s,
> as their translation may not fit within that width, in which case
> they may want to make these columns wider?
As far as I understand, %12s means that the argument printed will have
a minimum length of 12 columns. So if the translation of 'stage' is
longer than 12 it will be printed fully no matter what. Though in that
case, the header will not be align correctly anymore:
my $status_fmt = '%12s %12s %s';
123456789abcdefghijkl unstaged caminho
1: unchanged +1/-0 git-add--interactive.perl
2: unchanged +4226/-3152 po/git.pot
3: unchanged +11542/-10426 po/pt_PT.po
my $status_fmt = '%12s %8s %s';
123456789abcdefghijkl unstaged caminho
1: unchanged +2/-1 git-add--interactive.perl
2: unchanged +4226/-3152 po/git.pot
3: unchanged +11542/-10426 po/pt_PT.po
For reference in C locale (my $status_fmt = '%12s %12s %s';)
staged unstaged path
1: +4/-4 nothing git-add--interactive.perl
2: unchanged +4232/-3150 po/git.pot
3: unchanged +11572/-10448 po/pt_PT.po
I did not contemplate this issue before, but I think allowing a
translator to tweak $status_fmt would not be enough to properly align
the header if the translation is longer than 12 columns.
Maybe a lazy solution would be to add a TRANSLATOR: comment asking to
fit the translation of those words in 12 columns, but that would be
unpractical because 'stage' and 'unstage' can occur alone, like they do
here, in other place in the future, without having that length
restriction.
I think the real fix would be to find the longer column and use that
length for the remaining rows. I will try to do that if I can.
I also forgot to mark strings 'unchanged' and 'nothing' that are
displayed on that status. I will mark then in the next re-roll.
> > prompt_yesno(
> > - 'Your edited hunk does not apply. Edit again '
> > - . '(saying "no" discards!) [y/n]? '
> > + # TRANSLATORS: do not translate [y/n]
> > + # The program will only accept that input
> > + # at this point.
> > + __('Your edited hunk does not apply. Edit again '
> > + . '(saying "no" discards!) [y/n]? ')
>
> Not just [y/n], but "no" in "saying no discards!" also needs to
> stay, no? I wonder if it is a good idea to lose the TRANSLATORS
> comment by ejecting "[y/n]" outside the "__()" construct here.
I fear that ejecting "[y/n]" would not be good for right-to-left
languages since "[y/n]" would be the first thing a user of those
languages would read followed by the actual question. I feel the same
for other instances of this in the present patch series.
^ permalink raw reply
* [PATCH v2] gpg-interface: use more status letters
From: Michael J Gruber @ 2016-09-28 14:24 UTC (permalink / raw)
To: git; +Cc: Alex
In-Reply-To: <xmqqk2dxp84i.fsf@gitster.mtv.corp.google.com>
According to gpg2's doc/DETAILS:
"For each signature only one of the codes GOODSIG, BADSIG, EXPSIG,
EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted."
gpg1 ("classic") behaves the same (although doc/DETAILS
differs).
Currently, we parse gpg's status output for GOODSIG, BADSIG and trust
information and translate that into status codes G, B, U, N for the %G?
format specifier.
git-verify-* returns success in the GOODSIG case only. This is somewhat in
disagreement with gpg, which considers the first 5 of the 6 above as VALIDSIG,
but we err on the very safe side.
Introduce additional status codes E, X, R for ERRSIG, EXP*SIG, REVKEYSIG
so that a user of %G? gets more information about the absence of a 'G'
on first glance.
Requested-by: Alex <agrambot@gmail.com>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Changes in v2:
- Use GNUPGHOME="$HOME/gnupg-home-not-used" just like in other tests (lib).
- Do not parse for signer UID in the ERRSIG case (and test that we do not).
- Retreat "rather" addition from the doc: good/valid are terms that we use
differently from gpg anyways.
Documentation/pretty-formats.txt | 9 +++++++--
gpg-interface.c | 13 ++++++++++---
pretty.c | 3 +++
t/t7510-signed-commit.sh | 12 +++++++++++-
4 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index a942d57..c28ff2b 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -143,8 +143,13 @@ ifndef::git-rev-list[]
- '%N': commit notes
endif::git-rev-list[]
- '%GG': raw verification message from GPG for a signed commit
-- '%G?': show "G" for a good (valid) signature, "B" for a bad signature,
- "U" for a good signature with unknown validity and "N" for no signature
+- '%G?': show "G" for a good (valid) signature,
+ "B" for a bad signature,
+ "U" for a good signature with unknown validity,
+ "X" for a good expired signature, or good signature made by an expired key,
+ "R" for a good signature made by a revoked key,
+ "E" if the signature cannot be checked (e.g. missing key)
+ and "N" for no signature
- '%GS': show the name of the signer for a signed commit
- '%GK': show the key used to sign a signed commit
- '%gD': reflog selector, e.g., `refs/stash@{1}` or
diff --git a/gpg-interface.c b/gpg-interface.c
index 8672eda..6999e7b 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -33,6 +33,10 @@ static struct {
{ 'B', "\n[GNUPG:] BADSIG " },
{ 'U', "\n[GNUPG:] TRUST_NEVER" },
{ 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
+ { 'E', "\n[GNUPG:] ERRSIG "},
+ { 'X', "\n[GNUPG:] EXPSIG "},
+ { 'X', "\n[GNUPG:] EXPKEYSIG "},
+ { 'R', "\n[GNUPG:] REVKEYSIG "},
};
void parse_gpg_output(struct signature_check *sigc)
@@ -54,9 +58,12 @@ void parse_gpg_output(struct signature_check *sigc)
/* The trust messages are not followed by key/signer information */
if (sigc->result != 'U') {
sigc->key = xmemdupz(found, 16);
- found += 17;
- next = strchrnul(found, '\n');
- sigc->signer = xmemdupz(found, next - found);
+ /* The ERRSIG message is not followed by signer information */
+ if (sigc-> result != 'E') {
+ found += 17;
+ next = strchrnul(found, '\n');
+ sigc->signer = xmemdupz(found, next - found);
+ }
}
}
}
diff --git a/pretty.c b/pretty.c
index 493edb0..39a36cd 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1232,8 +1232,11 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
switch (c->signature_check.result) {
case 'G':
case 'B':
+ case 'E':
case 'U':
case 'N':
+ case 'X':
+ case 'R':
strbuf_addch(sb, c->signature_check.result);
}
break;
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 6e839f5..9f487f9 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -190,7 +190,7 @@ test_expect_success GPG 'show bad signature with custom format' '
test_cmp expect actual
'
-test_expect_success GPG 'show unknown signature with custom format' '
+test_expect_success GPG 'show untrusted signature with custom format' '
cat >expect <<-\EOF &&
U
61092E85B7227189
@@ -200,6 +200,16 @@ test_expect_success GPG 'show unknown signature with custom format' '
test_cmp expect actual
'
+test_expect_success GPG 'show unknown signature with custom format' '
+ cat >expect <<-\EOF &&
+ E
+ 61092E85B7227189
+
+ EOF
+ GNUPGHOME="$HOME/gnupg-home-not-used" git log -1 --format="%G?%n%GK%n%GS" eighth-signed-alt >actual &&
+ test_cmp expect actual
+'
+
test_expect_success GPG 'show lack of signature with custom format' '
cat >expect <<-\EOF &&
N
--
2.10.0.527.gbcb6904
^ permalink raw reply related
* Re: [PATCH v2 01/11] i18n: add--interactive: mark strings for translation
From: Duy Nguyen @ 2016-09-28 14:29 UTC (permalink / raw)
To: Vasco Almeida
Cc: Junio C Hamano, Git Mailing List, Jiang Xin,
Ævar Arnfjörð Bjarmason, David Aguilar
In-Reply-To: <1475066620.3257.12.camel@sapo.pt>
On Wed, Sep 28, 2016 at 7:43 PM, Vasco Almeida <vascomalmeida@sapo.pt> wrote:
> A Dom, 25-09-2016 às 15:52 -0700, Junio C Hamano escreveu:
>> > @@ -252,7 +253,7 @@ sub list_untracked {
>> > }
>> >
>> > my $status_fmt = '%12s %12s %s';
>> > -my $status_head = sprintf($status_fmt, 'staged', 'unstaged', 'path');
>> > +my $status_head = sprintf($status_fmt, __('staged'), __('unstaged'), __('path'));
>>
>> Wouldn't it make sense to allow translators to tweak $status_fmt if
>> you are allowing the earlier elements that are formatted with %12s,
>> as their translation may not fit within that width, in which case
>> they may want to make these columns wider?
>
> As far as I understand, %12s means that the argument printed will have
> a minimum length of 12 columns. So if the translation of 'stage' is
> longer than 12 it will be printed fully no matter what. Though in that
> case, the header will not be align correctly anymore:
> for other instances of this in the present patch series.
It's 12 bytes, not columns (unless perl understands input string's
encoding, which I doubt). Think about multi-byte encodings like utf-8,
where three letters (or "columns") do not necessary mean three bytes.
The result is most likely unaligned in that case.
--
Duy
^ permalink raw reply
* Re: [PATCH v2] gpg-interface: use more status letters
From: Ramsay Jones @ 2016-09-28 15:10 UTC (permalink / raw)
To: Michael J Gruber, git; +Cc: Alex
In-Reply-To: <c4777ef68059034d7ad4697a06bba3cabbdc9265.1475053649.git.git@drmicha.warpmail.net>
On 28/09/16 15:24, Michael J Gruber wrote:
> According to gpg2's doc/DETAILS:
> "For each signature only one of the codes GOODSIG, BADSIG, EXPSIG,
> EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted."
>
> gpg1 ("classic") behaves the same (although doc/DETAILS
> differs).
>
> Currently, we parse gpg's status output for GOODSIG, BADSIG and trust
> information and translate that into status codes G, B, U, N for the %G?
> format specifier.
>
> git-verify-* returns success in the GOODSIG case only. This is somewhat in
> disagreement with gpg, which considers the first 5 of the 6 above as VALIDSIG,
> but we err on the very safe side.
>
> Introduce additional status codes E, X, R for ERRSIG, EXP*SIG, REVKEYSIG
> so that a user of %G? gets more information about the absence of a 'G'
> on first glance.
>
> Requested-by: Alex <agrambot@gmail.com>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> Changes in v2:
>
> - Use GNUPGHOME="$HOME/gnupg-home-not-used" just like in other tests (lib).
> - Do not parse for signer UID in the ERRSIG case (and test that we do not).
> - Retreat "rather" addition from the doc: good/valid are terms that we use
> differently from gpg anyways.
>
> Documentation/pretty-formats.txt | 9 +++++++--
> gpg-interface.c | 13 ++++++++++---
> pretty.c | 3 +++
> t/t7510-signed-commit.sh | 12 +++++++++++-
> 4 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index a942d57..c28ff2b 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -143,8 +143,13 @@ ifndef::git-rev-list[]
> - '%N': commit notes
> endif::git-rev-list[]
> - '%GG': raw verification message from GPG for a signed commit
> -- '%G?': show "G" for a good (valid) signature, "B" for a bad signature,
> - "U" for a good signature with unknown validity and "N" for no signature
> +- '%G?': show "G" for a good (valid) signature,
> + "B" for a bad signature,
> + "U" for a good signature with unknown validity,
> + "X" for a good expired signature, or good signature made by an expired key,
Hmm, this looks odd. Would the following:
"X" for a good signature made with an expired key,
mean something different?
ATB,
Ramsay Jones
^ permalink raw reply
* [PATCH] http: Control GSSAPI credential delegation.
From: Petr Stodulka @ 2016-09-28 16:05 UTC (permalink / raw)
To: git; +Cc: pstodulk
Delegation of credentials is disabled by default in libcurl since
version 7.21.7 due to security vulnerability CVE-2011-2192. Which
makes troubles with GSS/kerberos authentication where delegation
of credentials is required. This can be changed with option
CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter
since libcurl version 7.22.0.
This patch provides new configuration variable http.delegation
which corresponds to curl parameter "--delegation" (see man 1 curl).
The following values are supported:
* none (default).
* policy
* always
Signed-off-by: Petr Stodulka <pstodulk@redhat.com>
---
Documentation/config.txt | 14 ++++++++++++++
http.c | 32 ++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index e78293b..a179474 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1736,6 +1736,20 @@ http.emptyAuth::
a username in the URL, as libcurl normally requires a username for
authentication.
+http.delegation::
+ Control GSSAPI credential delegation. The delegation is disabled
+ by default in libcurl since version 7.21.7. Set parameter to tell
+ the server what it is allowed to delegate when it comes to user
+ credentials. Used with GSS/kerberos. Possible values are:
++
+--
+* `none` - Don't allow any delegation.
+* `policy` - Delegates if and only if the OK-AS-DELEGATE flag is set in the
+ Kerberos service ticket, which is a matter of realm policy.
+* `always` - Unconditionally allow the server to delegate.
+--
+
+
http.extraHeader::
Pass an additional HTTP header when communicating with a server. If
more than one such entry exists, all of them are added as extra
diff --git a/http.c b/http.c
index 82ed542..5f8fab3 100644
--- a/http.c
+++ b/http.c
@@ -90,6 +90,18 @@ static struct {
* here, too
*/
};
+#if LIBCURL_VERSION_NUM >= 0x071600
+static const char *curl_deleg;
+static struct {
+ const char *name;
+ long curl_deleg_param;
+} curl_deleg_levels[] = {
+ { "none", CURLGSSAPI_DELEGATION_NONE },
+ { "policy", CURLGSSAPI_DELEGATION_POLICY_FLAG },
+ { "always", CURLGSSAPI_DELEGATION_FLAG },
+};
+#endif
+
static struct credential proxy_auth = CREDENTIAL_INIT;
static const char *curl_proxyuserpwd;
static const char *curl_cookie_file;
@@ -323,6 +335,10 @@ static int http_options(const char *var, const char *value, void *cb)
return 0;
}
+ if (!strcmp("http.delegation", var)) {
+ return git_config_string(&curl_deleg, var, value);
+ }
+
if (!strcmp("http.pinnedpubkey", var)) {
#if LIBCURL_VERSION_NUM >= 0x072c00
return git_config_pathname(&ssl_pinnedkey, var, value);
@@ -629,6 +645,22 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
#endif
+#if LIBCURL_VERSION_NUM >= 0x071600
+ if (curl_deleg) {
+ int i;
+ for (i = 0; i < ARRAY_SIZE(curl_deleg_levels); i++) {
+ if (!strcmp(curl_deleg, curl_deleg_levels[i].name)) {
+ curl_easy_setopt(result, CURLOPT_GSSAPI_DELEGATION,
+ curl_deleg_levels[i].curl_deleg_param);
+ break;
+ }
+ }
+ if (i == ARRAY_SIZE(curl_deleg_levels))
+ warning("Unknown delegation method '%s': using default",
+ curl_deleg);
+ }
+#endif
+
if (http_proactive_auth)
init_curl_http_auth(result);
--
2.5.5
^ permalink raw reply related
* Re: [PATCH v2 01/11] i18n: add--interactive: mark strings for translation
From: Junio C Hamano @ 2016-09-28 16:59 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
David Aguilar
In-Reply-To: <1475066620.3257.12.camel@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> As far as I understand, %12s means that the argument printed will have
> a minimum length of 12 columns. So if the translation of 'stage' is
> longer than 12 it will be printed fully no matter what. Though in that
> case, the header will not be align correctly anymore:
Exactly. That was where my suggestion comes from. In such a case
you may want to raise these numbers so that the fixed part
(i.e. header that you are letting the translators insert their
version of these words) would fit.
As Duy points out in his response to your message, that widening
further needs to take into account how many display columns each
translated words and phrases occupies, not just its byte length.
^ permalink raw reply
* RE: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout
From: Ben Peart @ 2016-09-28 17:02 UTC (permalink / raw)
To: git
Cc: Ben Peart, pclouds, Jeff Hostetler, philipoakley,
'Junio C Hamano'
Resending
> -----Original Message-----
> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
> Behalf Of Philip Oakley
> Sent: Saturday, September 24, 2016 3:31 PM
> To: Junio C Hamano <gitster@pobox.com>
> Cc: Ben Peart <Ben.Peart@microsoft.com>; pclouds@gmail.com;
> git@vger.kernel.org
> Subject: Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial
> checkout
>
> Hi Junio,
>
> From: "Junio C Hamano" <gitster@pobox.com>
> > "Philip Oakley" <philipoakley@iee.org> writes:
> >
> >>> > >"git checkout -b foo" (without -f -m or <start_point>) is defined
> >>> > >in the manual as being a shortcut for/equivalent to:
> >>> > >
> >>> > > (1a) "git branch foo"
> >>> > > (1b) "git checkout foo"
> >>> > >
> >>> > >However, it has been our experience in our observed use cases and
> >>> > >all the existing git tests, that it can be treated as equivalent
to:
> >>> > >
> >>> > > (2a) "git branch foo"
> >>> > > (2b) "git symbolic-ref HEAD refs/heads/foo"
> >>> > >...
> >>> > >
> >>> > I am still not sure if I like the change of what "checkout -b" is
> >>> > this late in the game, though.
> >>>
> >>> ...
> >>> That said, you're much more on the frontline of receiving negative
> >>> feedback about doing that than I am. :) How would you like to
> >>> proceed?
> >>
> >> I didn't see an initial confirmation as to what the issue really was.
> >> You indicated the symptom ('a long checkout time'), but then we
> >> missed out on hard facts and example repos, so that the issue was
> >> replicable.
> >
> > I took it as a given, trivial and obvious optimization opportunity,
> > that it is wasteful having to traverse two trees to consolidate and
> > reflect their differences into the working tree when we know upfront
> > that these two trees are identical, no matter what the overhead for
> > doing so is.
>
> I agree, and I believe Ben agrees.
>
Correct. In my original patch request I put more specific information on
the impact this optimization has in our specific case (reducing the cost
from 166 seconds to 16 seconds).
> >
> >> At the moment there is the simple workaround of an alias that
> >> executes that two step command dance to achieve what you needed, and
> >> Junio has outlined the issues he needed to be covered from his
> >> maintainer perspective (e.g. the detection of sparse checkouts).
> >> Confirming the root causes would help in setting a baseline.
> >>
> >> I hope that is of help - I'd seen that the discussion had gone quiet.
> >
> > Some of the problems I have are:
> >
> > (1) "git checkout -b NEW", "git checkout", "git checkout HEAD^0"
> > and "git checkout HEAD" (no other parameters to any of them)
> > ought to give identical index and working tree. It is too
> > confusing to leave subtly different results that will lead to
> > hard to diagnose bugs for only one of them.
> >
> > (2) The proposed log message talks only about "performance
> > optimization",
>
> > while the purpose of the change is more
> > about
> > changing the definition
>
> Here I think is the misunderstanding. His purpose is NOT to change the
> definition (IIUC). As I read the message you reference below (and Ben's
other
> messages), I understood that he was trying to achieve what you said (i.e.
> optimise the trivial and obvious opportunity) of selecting for the common
> case (underlying conditions) where the two command sequences are
> identical. If the selected case / conditions is not identical then it is
defined
> wrongly...
>
> I suspect that it was Ben's 'soft' explanation that allowed the discussion
to
> diverge.
>
I'm unaccustomed to doing reviews like this via email so have been
struggling with how to most effectively communicate about the proposed
change. I appreciate any help and understanding as I go through this
for the first time.
My intention was not to change the users expected results which
I believe are to "create a new branch and switch to it." We reinforce
that expectation with the output of the command which completes
with the text "Switched to a new branch 'foo'"
>
> > of what "git checkout -b
> > NEW" is from
> > "git branch NEW && git checkout NEW" to "git branch NEW && git
> > symbolic-ref HEAD refs/heads/NEW". The explanation in a Ben's
> > later message <007401d21278$445eba80$cd1c2f80$@gmail.com> does
> > a much better job contrasting the two.
> >
> > (3) I identified only one difference as an example sufficient to
> > point out why the patch provided is not a pure optimization but
> > behaviour change. Fixing that example alone to avoid change in
> > the behaviour is trivial (see if the "info/sparse-checkout"
> > file is present and refrain from skipping the proper checkout),
>
> This is probably the point Ben needs to take on board to narrow the
> conditions down. There may be others.
>
The fact that "git checkout -b NEW" updates the index and as a
result reflects any changes in the sparse-checkout and the issue
Junio pointed out earlier about not calling show_local_changes
at the end of merge_working_tree are the only difference in behavior
I am aware of. Both of these are easily rectified.
That said, given we are skipping huge amounts of work by no longer
merging the commit trees, generating a new index, and merging the
local modifications in the working tree, it is possible that there are
other behavior changes I'm just not aware of.
> > but a much larger problem is that I do not know (and Ben does
> > not, I suspect) know what other behaviour changes the patch is
> > introducing, and worse, the checks are sufficiently dense too
> > detailed and intimate to the implementation of unpack_trees()
> > that it is impossible for anybody to make sure the exceptions
> > defined in this patch and updates to other parts of the system
> > will be kept in sync.
>
> I did not believe he was proposing such a change to behaviour, hence his
> difficulty in responding (or at least that is my perception). I.e. he was
> digging a hole in the wrong place.
>
> It is possible that he had accidentally introduced a behavious change, and
> having failed to explictly say "This patch (should) produces no behavious
> change", which then continued to re-inforce the misunderstanding.
>
> >
> > So my inclination at this point, unless we see somebody invents a
> > clever way to solve (3), is that any change that violates (1),
> > i.e. as long as the patch does "Are we doing '-b NEW'? Then we do
> > something subtly different", is not acceptable, and solving (3) in a
> > maintainable way smells like quite a hard thing to do. But it would
> > be ideal if (3) is solved cleanly, as we will then not have to worry
> > about changing behaviour at all and can apply the optimization for
> > all of the four cases equally. As a side effect, that approach
> > would solve problem (2) above.
> >
> > If we were to punt on keeping the sanity (1) and introduce a subtly
> > different "create a new branch and point the HEAD at it", an easier
> > way out may be be one of
> >
> > 1. a totally new command, e.g. "git branch-switch NEW" that takes
> > only a single argument and no other "checkout" options, or
> >
> > 2. a new option to "git checkout" that takes _ONLY_ a single
> > argument and incompatible with any other option or command line
> > argument, or
> >
> > 3. an alias that does "git branch" followed by "git symbolic-ref".
> >
> > Neither of the first two sounds palatable, though.
>
> It will need Ben to come back and clarify, if he did, or did not, want any
> behaviour change (beyond speed of action;-)
>
There is a subtlety here in what is meant by "any behavior change."
I did not want to change the users expectations of what this command
is used for. The only noticeable behavior change should only be that it
sped up by an order of magnitude.
To get that speed up, there is a change in behavior from git's
perspective as it is no longer doing a bunch of work it used to do
which is what is saving the time.
I was aware that skipping the commit merge/new index/merge working
tree meant that "git checkout NEW" would no longer update these to
reflect any potential changes to the sparse-checkout file.
To determine if this would change the results the user was *expecting*,
I searched the web and found that all the instructions I could locate
that taught people how to update the index/working tree after
making changes to the sparse-checkout file instructed them to use
"git read-tree -mu HEAD." I didn't find any that told people to use
"git checkout -b NEW"
Finally, when I made the optimization to skip these steps I then
verified that the test suite still passed all tests. I realize that
there is not 100% coverage of tests but I thought it was a good
indication that none of them were impacted by this optimization.
I've tried to think of a way to solve (3) in a more maintainable way
but have not been able to come up with anything. Ultimately,
to ensure are only applying the optimization in this specific case,
we have to test to make sure other options don't require the extra
steps. I'm open to suggestions!
I'm going to be out for the next 2 weeks so will be unable to respond
to activity on the thread but a co-worker who has been involved will
be responsive to feedback and rolling any new versions of the patch.
Thanks,
Ben
^ permalink raw reply
* Re: [PATCH] http: Control GSSAPI credential delegation.
From: Petr Stodulka @ 2016-09-28 17:03 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <1475078752-31195-1-git-send-email-pstodulk@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 537 bytes --]
On 28.9.2016 18:05, Petr Stodulka wrote:
> Delegation of credentials is disabled by default in libcurl since
> version 7.21.7 due to security vulnerability CVE-2011-2192. Which
> makes troubles with GSS/kerberos authentication where delegation
> of credentials is required. This can be changed with option
> CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter
> since libcurl version 7.22.0.
Correction:
Which makes troubles with GSS/kerberos authentication when delegation
of credentials is required.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] http: Control GSSAPI credential delegation.
From: Jeff King @ 2016-09-28 17:16 UTC (permalink / raw)
To: Petr Stodulka; +Cc: git
In-Reply-To: <1475078752-31195-1-git-send-email-pstodulk@redhat.com>
On Wed, Sep 28, 2016 at 06:05:52PM +0200, Petr Stodulka wrote:
> Delegation of credentials is disabled by default in libcurl since
> version 7.21.7 due to security vulnerability CVE-2011-2192. Which
> makes troubles with GSS/kerberos authentication where delegation
> of credentials is required. This can be changed with option
> CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter
> since libcurl version 7.22.0.
I don't have any real knowledge of GSSAPI, so I'll refrain from
commenting on that aspect. But I did notice one mechanical issue:
> +#if LIBCURL_VERSION_NUM >= 0x071600
> +static const char *curl_deleg;
> +static struct {
> + const char *name;
> + long curl_deleg_param;
> +} curl_deleg_levels[] = {
> + { "none", CURLGSSAPI_DELEGATION_NONE },
> + { "policy", CURLGSSAPI_DELEGATION_POLICY_FLAG },
> + { "always", CURLGSSAPI_DELEGATION_FLAG },
> +};
> +#endif
We only declare the curl_deleg variable if we have a new-enough curl.
But...
> @@ -323,6 +335,10 @@ static int http_options(const char *var, const char *value, void *cb)
> return 0;
> }
>
> + if (!strcmp("http.delegation", var)) {
> + return git_config_string(&curl_deleg, var, value);
> + }
> +
...here we try to use it regardless. I think you want another #ifdef,
and probably to warn the user in the #else block (similar to what the
http.pinnedpubkey code does).
-Peff
^ permalink raw reply
* Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules
From: Brandon Williams @ 2016-09-28 17:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqwphxm7av.fsf@gitster.mtv.corp.google.com>
On 09/27, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
> > we seem to avoid using "tr", even though q_to_cr and others do use
> > it. I wonder if we had some portability issues with passing NUL
> > through tr or something?
> >
> > ... digs and finds e85fe4d8 ("more tr portability test script
> > fixes", 2008-03-12)
> >
> > So use something like
> >
> > perl -pe 'y/\012/\000/' <<\-EOF
> > ...
> > EOF
> >
> > instead, perhaps?
>
> I actually think it would make more sense to add
>
> lf_to_nul () {
> perl -pe 'y/\012/\000/'
> }
>
> to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
> this route.
Turns out this function already exists in test-lib-functions.sh
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 00/11] Resumable clone
From: Junio C Hamano @ 2016-09-28 17:32 UTC (permalink / raw)
To: Eric Wong; +Cc: Kevin Wern, git
In-Reply-To: <xmqqshslkndk.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>>> git clone --resume <resumable_work_or_git_dir>
>>
>> I think calling "git fetch" should resume, actually.
>> It would reduce the learning curve and seems natural to me:
>> "fetch" is jabout grabbing whatever else appeared since the
>> last clone/fetch happened.
>
> I hate say this but it sounds to me like a terrible idea. At that
> point when you need to resume, there is not even ref for "fetch" to
> base its incremental work off of. It is better to keep the knowledge
> of this "priming" dance inside "clone". Hopefully the original "clone"
> whose connection was disconnected in the middle would automatically
> attempt resuming and "clone --resume" would not be as often as needed.
After sleeping on this, I want to take the above back.
I think teaching "git fetch" about the "resume" part makes tons of
sense.
What "git clone" should have been was:
* Parse command line arguments;
* Create a new repository and go into it; this step would
require us to have parsed the command line for --template,
<directory>, --separate-git-dir, etc.
* Talk to the remote and do get_remote_heads() aka ls-remote
output;
* Decide what fetch refspec to use, which alternate object store
to borrow from; this step would require us to have parsed the
command line for --reference, --mirror, --origin, etc;
--- we'll insert something new here ---
* Issue "git fetch" with the refspec determined above; this step
would require us to have parsed the command line for --depth, etc.
* Run "git checkout -b" to create an initial checkout; this step
would require us to have parsed the command line for --branch,
etc.
Even though the current code conceptually does the above, these
steps are not cleanly separated as such. I think our update to gain
"resumable clone" feature on the client side need to start by
refactoring the current code, before learning "resumable clone", to
look like the above.
Once we do that, we can insert an extra step before the step that
runs "git fetch" to optionally [*1*] grab the extra piece of
information Kevin's "prime-clone" service produces [*2*], and store
it in the "new repository" somewhere [*3*].
And then, as you suggested, an updated "git fetch" can be taught to
notice the priming information left by the previous step, and use it
to attempt to download the pack until success, and to index that
pack to learn the tips that can be used as ".have" entries in the
request. From the original server's point of view, this fetch
request would "want" the same set of objects, but would appear as
an incremental update.
Of course, the final step that happens in "git clone", i.e. the
initial checkout, needs to be done somehow, if your user decides to
resume with "git fetch", as "git fetch" _never_ touches the working
tree. So for that purpose, the primary end-user facing interface
may still have to be "git clone --resume <dir>". That would
probably skip all four steps in the above sequence, the new
"download priming information" step and go directly to the step that
runs "git fetch".
I do agree that is a much better design, and the crucial design
decision that makes it a better design is your making "git fetch"
aware of this "ah, we have the instruction left in this repository
how to prime its object store" information.
Thanks.
[Footnotes]
*1* It is debatable if it would be an overall win to use the "first
prime by grabbing a large packfile" clone if we are doing
shallow or single-branch clone, hence "optionally". It is
important to notice that we already have enough information to
base the decision at this point in the above sequence.
*2* As I said, I do not think it needs to be a separate new service,
and I suspect it may be a better design to carry it over the
protocol extension. At this point in the above sequence, we
have done an equivalent of ls-remote and if we designed a
protocol extension to carry the information we should already
have it. If we use a separate new service, we can of course
make a separate connection to ask about "prime-clone"
information. The way this piece of information is transmitted
is of secondary importance.
*3* In addition to the "prime-clone" information, we may need to
store some information that is only known to "clone" (perhaps
because it was given from the command line) to help the final
"checkout -b" step to know what to checkout around here, in case
the next "fetch" step is interrupted and killed.
^ permalink raw reply
* Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout
From: Junio C Hamano @ 2016-09-28 17:52 UTC (permalink / raw)
To: Ben Peart; +Cc: git, Ben Peart, pclouds, Jeff Hostetler, philipoakley
In-Reply-To: <004d01d219aa$0a941fa0$1fbc5ee0$@gmail.com>
"Ben Peart" <peartben@gmail.com> writes:
> The fact that "git checkout -b NEW" updates the index and as a
> result reflects any changes in the sparse-checkout and the issue
> Junio pointed out earlier about not calling show_local_changes
> at the end of merge_working_tree are the only difference in behavior
> I am aware of. Both of these are easily rectified.
>
> That said, given we are skipping huge amounts of work by no longer
> merging the commit trees, generating a new index, and merging the
> local modifications in the working tree, it is possible that there are
> other behavior changes I'm just not aware of.
That is OK. It is not ok to leave such bugs at the end of the
development before the topic is merged to 'master' to be delivered
to the end users, but you do not have to fight alone to produce a
perfect piece of code with your first attempt. That's what the
reviews and testing period are for.
If you are shooting for the same behaviour, then that is much better
than "make 'checkout -b NEW' be equivalent to a sequence of
update-ref && symbolic-ref, which is different from others", which
was the second explanation you gave earlier. I am much happier with
that goal.
But if that is the case, I really do not see any point of singling
out "-b NEW" case. The following property MUST be kept:
(1) "git checkout -b NEW", "git checkout", "git checkout HEAD^0"
and "git checkout HEAD" (no other parameters to any of them)
ought to give identical index and working tree. It is too
confusing to leave subtly different results that will lead to
hard to diagnose bugs for only one of them.
That would make the "do we skip unpack_trees() call?" decision a lot
simpler to make, I would suspect. We only need to see "are the two
trees we would fed unpack_trees() the same as HEAD's tree?" and do
not have to look at new_branch and other irrelevant things at all.
What happens in the ref namespace is immaterial, as making or
skipping an unpack_trees() call would not affect anything other than
the resulting index and the working tree. If we want to keep that
sparse-checkout wart, we would also need to see if the control file
sparse-checkout keeps in $GIT_DIR/ exists, but the result will be
much simpler set of rules, and would hopefully help remove the "the
optimization kicks in following logic that is an unreviewable-mess"
issue.
^ permalink raw reply
* Re: [PATCH 10/11] run command: add RUN_COMMAND_NO_STDOUT
From: Junio C Hamano @ 2016-09-28 17:54 UTC (permalink / raw)
To: Kevin Wern; +Cc: git
In-Reply-To: <20160928044622.GE3762@kwern-HP-Pavilion-dv5-Notebook-PC>
Kevin Wern <kevin.m.wern@gmail.com> writes:
> On Fri, Sep 16, 2016 at 04:07:00PM -0700, Junio C Hamano wrote:
>> Kevin Wern <kevin.m.wern@gmail.com> writes:
>>
>> > Add option RUN_COMMAND_NO_STDOUT, which sets no_stdout on a child
>> > process.
>> >
>> > This will be used by git clone when calling index-pack on a downloaded
>> > packfile.
>>
>> If it is just one caller, would't it make more sense for that caller
>> set no_stdout explicitly itself?
>
> I based the calling code in do_index_pack on dissociate_from_references, which
> uses run_command_v_opt, so it never occured to me to do that. I thought it was
> just good, uniform style and encapsulation. Like how transport's methods and
> internals aren't really intended to be changed or accessed--unless it's through
> the APIs we create.
>
> However, I don't feel very strongly about this, so I'm okay with this change.
I am neutral and with no opinion. I may have offered a solution to
a problem that did not exist.
I just got an impression that you were apologetic for having to add
this option that is otherwise useless and tried to suggest a simpler
solution that does not involve such an addition.
^ permalink raw reply
* Re: [PATCH 3/3] docs/cvs-migration: mention cvsimport caveats
From: Junio C Hamano @ 2016-09-28 17:59 UTC (permalink / raw)
To: Eric S. Raymond; +Cc: Jeff King, git
In-Reply-To: <20160928001108.GA9120@thyrsus.com>
"Eric S. Raymond" <esr@thyrsus.com> writes:
> Jeff King <peff@peff.net>:
>> I am not qualified to write on the current state of
>> the art in CVS importing.
>
> I *am* qualified; cvs-fast-export has had a lot of work put into it by
> myself and others over the last five years. Nobody else is really
> working this problem anymore, not much else than cvs2git is even left
> standing at this point.
It sounds like you, as a better qualified person, would be in the
best position to send an update to the documentation to tell people
not to use older and unmaintained ones and guides them instead to a
newer and better tool.
... ah, I notice that peff said the same already.
I'd be fine with reviewing and applying such a patch.
Thanks.
^ permalink raw reply
* [PATCH v2] http: Control GSSAPI credential delegation.
From: Petr Stodulka @ 2016-09-28 18:01 UTC (permalink / raw)
To: git; +Cc: pstodulk
In-Reply-To: <20160928171610.pbghg4sk23vm4xnp@sigill.intra.peff.net>
Delegation of credentials is disabled by default in libcurl since
version 7.21.7 due to security vulnerability CVE-2011-2192. Which
makes troubles with GSS/kerberos authentication when delegation
of credentials is required. This can be changed with option
CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter
since libcurl version 7.22.0.
This patch provides new configuration variable http.delegation
which corresponds to curl parameter "--delegation" (see man 1 curl).
The following values are supported:
* none (default).
* policy
* always
Signed-off-by: Petr Stodulka <pstodulk@redhat.com>
---
Documentation/config.txt | 14 ++++++++++++++
http.c | 37 +++++++++++++++++++++++++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index e78293b..a179474 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1736,6 +1736,20 @@ http.emptyAuth::
a username in the URL, as libcurl normally requires a username for
authentication.
+http.delegation::
+ Control GSSAPI credential delegation. The delegation is disabled
+ by default in libcurl since version 7.21.7. Set parameter to tell
+ the server what it is allowed to delegate when it comes to user
+ credentials. Used with GSS/kerberos. Possible values are:
++
+--
+* `none` - Don't allow any delegation.
+* `policy` - Delegates if and only if the OK-AS-DELEGATE flag is set in the
+ Kerberos service ticket, which is a matter of realm policy.
+* `always` - Unconditionally allow the server to delegate.
+--
+
+
http.extraHeader::
Pass an additional HTTP header when communicating with a server. If
more than one such entry exists, all of them are added as extra
diff --git a/http.c b/http.c
index 82ed542..0c65639 100644
--- a/http.c
+++ b/http.c
@@ -90,6 +90,18 @@ static struct {
* here, too
*/
};
+#if LIBCURL_VERSION_NUM >= 0x071600
+static const char *curl_deleg;
+static struct {
+ const char *name;
+ long curl_deleg_param;
+} curl_deleg_levels[] = {
+ { "none", CURLGSSAPI_DELEGATION_NONE },
+ { "policy", CURLGSSAPI_DELEGATION_POLICY_FLAG },
+ { "always", CURLGSSAPI_DELEGATION_FLAG },
+};
+#endif
+
static struct credential proxy_auth = CREDENTIAL_INIT;
static const char *curl_proxyuserpwd;
static const char *curl_cookie_file;
@@ -323,6 +335,15 @@ static int http_options(const char *var, const char *value, void *cb)
return 0;
}
+ if (!strcmp("http.delegation", var)) {
+#if LIBCURL_VERSION_NUM >= 0x071600
+ return git_config_string(&curl_deleg, var, value);
+#else
+ warning(_("Delegation control is not supported with cURL < 7.22.0"));
+ return 0;
+#endif
+ }
+
if (!strcmp("http.pinnedpubkey", var)) {
#if LIBCURL_VERSION_NUM >= 0x072c00
return git_config_pathname(&ssl_pinnedkey, var, value);
@@ -629,6 +650,22 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
#endif
+#if LIBCURL_VERSION_NUM >= 0x071600
+ if (curl_deleg) {
+ int i;
+ for (i = 0; i < ARRAY_SIZE(curl_deleg_levels); i++) {
+ if (!strcmp(curl_deleg, curl_deleg_levels[i].name)) {
+ curl_easy_setopt(result, CURLOPT_GSSAPI_DELEGATION,
+ curl_deleg_levels[i].curl_deleg_param);
+ break;
+ }
+ }
+ if (i == ARRAY_SIZE(curl_deleg_levels))
+ warning("Unknown delegation method '%s': using default",
+ curl_deleg);
+ }
+#endif
+
if (http_proactive_auth)
init_curl_http_auth(result);
--
2.5.5
^ permalink raw reply related
* Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors
From: Junio C Hamano @ 2016-09-28 18:02 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Haggerty, David Turner, git, David Turner
In-Reply-To: <20160928085841.aoisson3fnuke47q@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> if (!dont_change_ref) {
> struct ref_transaction *transaction;
> - struct strbuf err = STRBUF_INIT;
> -
> - transaction = ref_transaction_begin(&err);
> - if (!transaction ||
> - ref_transaction_update(transaction, ref.buf,
> - sha1, forcing ? NULL : null_sha1,
> - 0, msg, &err) ||
> - ref_transaction_commit(transaction, &err))
> - die("%s", err.buf);
> +
> + transaction = ref_transaction_begin(&error_die);
> + ref_transaction_update(transaction, ref.buf,
> + sha1, forcing ? NULL : null_sha1,
> + 0, msg, &error_die);
> + ref_transaction_commit(transaction, &error_die);
> ref_transaction_free(transaction);
> - strbuf_release(&err);
> }
>
> if (real_ref && track)
>
> which is much shorter and to the point (it does rely on the called
> functions always calling report_error() and never just returning NULL or
> "-1", but that should be the already. If it isn't, we'd be printing
> "fatal: " with no message).
Yes but... grepping for die() got a lot harder, which may not be a
good thing.
I do like the flexibility such a mechanism offers, but
wrapping/hiding die in it is probably an example that the
flexibility went a bit too far.
^ permalink raw reply
* Re: [PATCH 10/11] run command: add RUN_COMMAND_NO_STDOUT
From: Kevin Wern @ 2016-09-28 18:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kevin Wern, git
In-Reply-To: <xmqqponnkiz7.fsf@gitster.mtv.corp.google.com>
On Wed, Sep 28, 2016 at 10:54:52AM -0700, Junio C Hamano wrote:
>
> I just got an impression that you were apologetic for having to add
> this option that is otherwise useless and tried to suggest a simpler
> solution that does not involve such an addition.
Sorry, to be clear, I meant I was ok with your suggestion. That's what I meant
by 'this change.'
^ permalink raw reply
* Re: [PATCH 00/11] Resumable clone
From: Junio C Hamano @ 2016-09-28 18:22 UTC (permalink / raw)
To: Eric Wong; +Cc: Kevin Wern, git
In-Reply-To: <xmqqy42cj5g1.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
> What "git clone" should have been was:
>
> * Parse command line arguments;
>
> * Create a new repository and go into it; this step would
> require us to have parsed the command line for --template,
> <directory>, --separate-git-dir, etc.
>
> * Talk to the remote and do get_remote_heads() aka ls-remote
> output;
>
> * Decide what fetch refspec to use, which alternate object store
> to borrow from; this step would require us to have parsed the
> command line for --reference, --mirror, --origin, etc;
>
> --- we'll insert something new here ---
>
> * Issue "git fetch" with the refspec determined above; this step
> would require us to have parsed the command line for --depth, etc.
>
> * Run "git checkout -b" to create an initial checkout; this step
> would require us to have parsed the command line for --branch,
> etc.
>
> Even though the current code conceptually does the above, these
> steps are not cleanly separated as such. I think our update to gain
> "resumable clone" feature on the client side need to start by
> refactoring the current code, before learning "resumable clone", to
> look like the above.
>
> Once we do that, we can insert an extra step before the step that
> runs "git fetch" to optionally [*1*] grab the extra piece of
> information Kevin's "prime-clone" service produces [*2*], and store
> it in the "new repository" somewhere [*3*].
>
> And then, as you suggested, an updated "git fetch" can be taught to
> notice the priming information left by the previous step, and use it
> to attempt to download the pack until success, and to index that
> pack to learn the tips that can be used as ".have" entries in the
> request. From the original server's point of view, this fetch
> request would "want" the same set of objects, but would appear as
> an incremental update.
Thinking about this even more, it probably makes even more sense to
move the new "learn prime info and store it in repository somewhere,
so that later re-invocation of 'git fetch' can take advantage of it"
step _into_ "git fetch". That would allow "git fetch" in a freshly
created empty repository take advantage of this feature for free.
The step that "git clone" internally drives "git fetch" would not
actually be done by spawning a separate process with run_command()
because we would want to reuse the connection we already have with
the server when "git clone" first talked to it to learn "ls-remote"
equivalent (i.e. transport_get_remote_refs()). I wonder if we can
do without this early "ls-remote"; that would further simplify
things by allowing us to just spawn "git fetch" internally.
^ permalink raw reply
* Re: [PATCH 2/3] diff-lib.c: enable --shift-ita in index_differs_from()
From: Junio C Hamano @ 2016-09-28 18:49 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20160928114348.1470-3-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> This function is basically "git diff --cached HEAD", It has three
> callers:
>
> - One in builtin/commit.c, which uses it to determine if the index is
> different from HEAD and go ahead making a new commit.
>
> - Two in sequencer.c, which use it to see if the index is dirty.
>
> In the first case, if ita entries are present, index_differs_from() may
> report "dirty". However at tree creation phase, ita entries are dropped
> and the result tree may look exactly the same as HEAD (assuming that
> nothing else is changed in index). This is what we need index_differs_from()
> for, to catch new empty commits. Enabling shift_ita in index_differs_from()
> fixes this.
>
> In the second case, the presence of ita entries are enough to say the
> index is dirty and not continue on. Make an explicit check for that
> before comparing index against HEAD (whether --shift-ita is present is
> irrelevant)
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
There are three callers to index_differs_from(), which asks "is the
index different from the HEAD". Because you want to change the
behaviour of the function for one of these callers while not
exposing its undesirable behaviour for the other two callers, you
guard the call to it with another call to a new helper function,
which needs to scan the entire index one more time.
It somehow sounds like backwards to me.
IOW, I wonder if it makes more sense to add a new interface to tell
the index_differs_from() function "I want you to use shift-ita
semantics" bit, and pass that when calling it from builtin/commit.c
while not toggling that bit on when the other two callers call it,
without introducing the has_ita_entries() helper function.
By the way, I think "shift" is a bit unclear name for the diffopt
field. The log message of [1/3] is totally unclear (it claims
"smaller and safer" without explaining what it exactly does and why
that is safer); the documentation update in it is slightly better in
that it lets intelligent readers to guess that the option is to
declare that ita entries do not yet exist in the index (hence, "git
diff" would say "that's a new file", while "git diff --cached" says
nothing about it). From that observation, I think a descriptive
phrase that is suitable for its name than "shift" needs to be found
in a short explanation of what it does: "treat ita as missing in the
index", e.g. "rev.diffopt.ita_is_missing = 1", perhaps?
Other than these small implementation details, I think I like the
direction these two patches are taking us (I haven't checked 3/3
yet).
Thanks.
^ permalink raw reply
* Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules
From: Junio C Hamano @ 2016-09-28 18:59 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <20160928172417.GA61176@google.com>
Brandon Williams <bmwill@google.com> writes:
>> I actually think it would make more sense to add
>>
>> lf_to_nul () {
>> perl -pe 'y/\012/\000/'
>> }
>>
>> to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
>> this route.
>
> Turns out this function already exists in test-lib-functions.sh
;-)
^ permalink raw reply
* Re: [PATCH] http: Control GSSAPI credential delegation.
From: Petr Stodulka @ 2016-09-28 18:19 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20160928171610.pbghg4sk23vm4xnp@sigill.intra.peff.net>
[-- Attachment #1.1: Type: text/plain, Size: 1821 bytes --]
On 28.9.2016 19:16, Jeff King wrote:
> On Wed, Sep 28, 2016 at 06:05:52PM +0200, Petr Stodulka wrote:
>
>> Delegation of credentials is disabled by default in libcurl since
>> version 7.21.7 due to security vulnerability CVE-2011-2192. Which
>> makes troubles with GSS/kerberos authentication where delegation
>> of credentials is required. This can be changed with option
>> CURLOPT_GSSAPI_DELEGATION in libcurl with set expected parameter
>> since libcurl version 7.22.0.
>
> I don't have any real knowledge of GSSAPI, so I'll refrain from
> commenting on that aspect. But I did notice one mechanical issue:
>
Me neither. I have just basic knowledge and I am not able to configure
virtual machine, which really need set delegation in libcurl (I need
just negotiation, which is in git possible, I guess since v2.8.0).
However, I discuss it with libcurl maintainer and he confirm that this
option can be required in some cases and this is what I need to do.
this already. I tested just setting of parameter in libcurl according
to description and nothing else seems broken. So anyone else who will
be able to test complete behaviour, where delegation is needed, is welcomed.
[snip]
> We only declare the curl_deleg variable if we have a new-enough curl.
> But...
>
>> @@ -323,6 +335,10 @@ static int http_options(const char *var, const char *value, void *cb)
>> return 0;
>> }
>>
>> + if (!strcmp("http.delegation", var)) {
>> + return git_config_string(&curl_deleg, var, value);
>> + }
>> +
>
> ...here we try to use it regardless. I think you want another #ifdef,
> and probably to warn the user in the #else block (similar to what the
> http.pinnedpubkey code does).
>
> -Peff
>
You are right. Thanks. I sent new version of patch with fix.
Petr
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox