[PATCH 3/3] pull: handle --verify-signatures for unborn branch
We usually just forward the --verify-signatures option along to
git-merge, and trust it to do the right thing. However, when we are on
an unborn branch (i.e., there is no HEAD yet), we handle this case
ourselves without even calling git-merge. And in this code path, we do
not respect the verification option at all.
It may be more maintainable in the long run to call git-merge for the
unborn case. That would fix this bug, as well as prevent similar ones in
the future. But unfortunately it's not easy to do. As t5520.3
demonstrates, there are some special cases that git-merge does not
handle, like "git pull .. master:master" (by the time git-merge is
invoked, we've overwritten the unborn HEAD).
So for now let's just teach git-pull to handle this feature.
Reported-by: Felix Eckhofer
Signed-off-by: Jeff King
---
builtin/pull.c| 11 +++
t/t5573-pull-verify-signatures.sh | 7 +++
2 files changed, 18 insertions(+)
diff --git a/builtin/pull.c b/builtin/pull.c
index 798ecf7faf..91616cf7d6 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -557,6 +557,17 @@ static int run_fetch(const char *repo, const char
**refspecs)
static int pull_into_void(const struct object_id *merge_head,
const struct object_id *curr_head)
{
+ if (opt_verify_signatures) {
+ struct commit *commit;
+
+ commit = lookup_commit(the_repository, merge_head);
+ if (!commit)
+ die(_("unable to access commit %s"),
+ oid_to_hex(merge_head));
+
+ verify_merge_signature(commit, opt_verbosity);
+ }
+
/*
* Two-way merge: we treat the index as based on an empty tree,
* and try to fast-forward to HEAD. This ensures we will not lose
diff --git a/t/t5573-pull-verify-signatures.sh
b/t/t5573-pull-verify-signatures.sh
index 747775c147..3e9876e197 100755
--- a/t/t5573-pull-verify-signatures.sh
+++ b/t/t5573-pull-verify-signatures.sh
@@ -78,4 +78,11 @@ test_expect_success GPG 'pull commit with bad signature with
--no-verify-signatu
git pull --ff-only --no-verify-signatures bad 2>pullerror
'
+test_expect_success GPG 'pull unsigned commit into unborn branch' '
+ git init empty-repo &&
+ test_must_fail \
+ git -C empty-repo pull --verify-signatures .. 2>pullerror &&
+ test_i18ngrep "does not have a GPG signature" pullerror
+'
+
test_done
--
2.19.1.1533.g982fead9fb
[PATCH 2/3] merge: handle --verify-signatures for unborn branch
When git-merge sees that we are on an unborn branch (i.e., there is no
HEAD), it follows a totally separate code path than the usual merge
logic. This code path does not know about verify_signatures, and so we
fail to notice bad or missing signatures.
This has been broken since --verify-signatures was added in efed002249
(merge/pull: verify GPG signatures of commits being merged, 2013-03-31).
In an ideal world, we'd unify the flow for this case with the regular
merge logic, which would fix this bug and avoid introducing similar
ones. But because the unborn case is so different, it would be a burden
on the rest of the function to continually handle the missing HEAD. So
let's just port the verification check to this special case.
Reported-by: Felix Eckhofer
Signed-off-by: Jeff King
---
builtin/merge.c| 4
t/t7612-merge-verify-signatures.sh | 7 +++
2 files changed, 11 insertions(+)
diff --git a/builtin/merge.c b/builtin/merge.c
index c677f9375e..be156b122d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1336,6 +1336,10 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
die(_("%s - not something we can merge"), argv[0]);
if (remoteheads->next)
die(_("Can merge only exactly one commit into empty
head"));
+
+ if (verify_signatures)
+ verify_merge_signature(remoteheads->item, verbosity);
+
remote_head_oid = >item->object.oid;
read_empty(remote_head_oid, 0);
update_ref("initial pull", "HEAD", remote_head_oid, NULL, 0,
diff --git a/t/t7612-merge-verify-signatures.sh
b/t/t7612-merge-verify-signatures.sh
index e2b1df817a..d99218a725 100755
--- a/t/t7612-merge-verify-signatures.sh
+++ b/t/t7612-merge-verify-signatures.sh
@@ -103,4 +103,11 @@ test_expect_success GPG 'merge commit with bad signature
with merge.verifySignat
git merge --no-verify-signatures $(cat forged.commit)
'
+test_expect_success GPG 'merge unsigned commit into unborn branch' '
+ test_when_finished "git checkout initial" &&
+ git checkout --orphan unborn &&
+ test_must_fail git merge --verify-signatures side-unsigned 2>mergeerror
&&
+ test_i18ngrep "does not have a GPG signature" mergeerror
+'
+
test_done
--
2.19.1.1533.g982fead9fb
[PATCH 1/3] merge: extract verify_merge_signature() helper
The logic to implement "merge --verify-signatures" is inline in
cmd_merge(), but this site misses some cases. Let's extract the logic
into a function so we can call it from more places.
We'll move it to commit.[ch], since one of the callers (git-pull) is
outside our source file. This function isn't all that general (after
all, its main function is to exit the program) but it's not worth trying
to fix that. The heavy lifting is done by check_commit_signature(), and
our purpose here is just sharing the die() logic. We'll mark it with a
comment to make that clear.
Signed-off-by: Jeff King
---
builtin/merge.c | 26 +-
commit.c| 26 ++
commit.h| 7 +++
3 files changed, 34 insertions(+), 25 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 4aa6071598..c677f9375e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1357,31 +1357,7 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
if (verify_signatures) {
for (p = remoteheads; p; p = p->next) {
- struct commit *commit = p->item;
- char hex[GIT_MAX_HEXSZ + 1];
- struct signature_check signature_check;
- memset(_check, 0, sizeof(signature_check));
-
- check_commit_signature(commit, _check);
-
- find_unique_abbrev_r(hex, >object.oid,
DEFAULT_ABBREV);
- switch (signature_check.result) {
- case 'G':
- break;
- case 'U':
- die(_("Commit %s has an untrusted GPG
signature, "
- "allegedly by %s."), hex,
signature_check.signer);
- case 'B':
- die(_("Commit %s has a bad GPG signature "
- "allegedly by %s."), hex,
signature_check.signer);
- default: /* 'N' */
- die(_("Commit %s does not have a GPG
signature."), hex);
- }
- if (verbosity >= 0 && signature_check.result == 'G')
- printf(_("Commit %s has a good GPG signature by
%s\n"),
- hex, signature_check.signer);
-
- signature_check_clear(_check);
+ verify_merge_signature(p->item, verbosity);
}
}
diff --git a/commit.c b/commit.c
index d566d7e45c..e1428aed6d 100644
--- a/commit.c
+++ b/commit.c
@@ -1100,7 +1100,33 @@ int check_commit_signature(const struct commit *commit,
struct signature_check *
return ret;
}
+void verify_merge_signature(struct commit *commit, int verbosity)
+{
+ char hex[GIT_MAX_HEXSZ + 1];
+ struct signature_check signature_check;
+ memset(_check, 0, sizeof(signature_check));
+
+ check_commit_signature(commit, _check);
+
+ find_unique_abbrev_r(hex, >object.oid, DEFAULT_ABBREV);
+ switch (signature_check.result) {
+ case 'G':
+ break;
+ case 'U':
+ die(_("Commit %s has an untrusted GPG signature, "
+ "allegedly by %s."), hex, signature_check.signer);
+ case 'B':
+ die(_("Commit %s has a bad GPG signature "
+ "allegedly by %s."), hex, signature_check.signer);
+ default: /* 'N' */
+ die(_("Commit %s does not have a GPG signature."), hex);
+ }
+ if (verbosity >= 0 && signature_check.result == 'G')
+ printf(_("Commit %s has a good GPG signature by %s\n"),
+ hex, signature_check.signer);
+ signature_check_clear(_check);
+}
void append_merge_tag_headers(struct commit_list *parents,
struct commit_extra_header ***tail)
diff --git a/commit.h b/commit.h
index 6c4428c593..a98cca635a 100644
--- a/commit.h
+++ b/commit.h
@@ -331,6 +331,13 @@ extern int remove_signature(struct strbuf *buf);
*/
extern int check_commit_signature(const struct commit *commit, struct
signature_check *sigc);
+/*
+ * Verify a single commit with check_commit_signature() and die() if it is not
+ * a good signature. This isn't really suitable for general use, but is a
+ * helper to implement consistent logic for pull/merge --verify-signatures.
+ */
+void verify_merge_signature(struct commit *commit, int verbose);
+
int compare_commits_by_commit_date(const void *a_, const void *b_, void
*unused);
int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_,
void *unused);
--
2.19.1.1533.g982fead9fb
[PATCH 0/3] fix pull/merge --verify-signature on an unborn branch
This bug was reported to the private security list, but I don't think it's easily exploitable, since merging or pulling into an unborn branch is pretty uncommon. The root of the issue in both commands is just that we handle unborn branches in a special code path that never learned about --verify-signatures. [1/3]: merge: extract verify_merge_signature() helper [2/3]: merge: handle --verify-signatures for unborn branch [3/3]: pull: handle --verify-signatures for unborn branch builtin/merge.c| 30 +- builtin/pull.c | 11 +++ commit.c | 26 ++ commit.h | 7 +++ t/t5573-pull-verify-signatures.sh | 7 +++ t/t7612-merge-verify-signatures.sh | 7 +++ 6 files changed, 63 insertions(+), 25 deletions(-) -Peff
Re: Understanding pack format
On Mon, Nov 05, 2018 at 09:23:45PM -0500, Farhan Khan wrote: > I am trying to identify where the content from a pack comes from. I > traced it back to sha1-file.c:read_object(), which will return the > 'content'. I want to know where the 'content' comes from, which seems > to come from sha1-file.c:oid_object_info_extended. This goes into > packfile.c:find_pack_entry(), but from here I get lost. I do not > understand what is happening. > > How does it retrieve the pack content? I am lost here. Please assist. > This is in the technical git documentation, but it was not clear. After find_pack_entry() tells us the object is in a pack, we end up in packed_object_info(). Depending what the caller is asking for, there are a couple different strategies (because we try to avoid loading the whole object if we don't need it). Probably the one you're interested in is just grabbing the content, which happens via cache_or_unpack_entry(). The cached case is less interesting, so try unpack_entry(), which is what actually reads the bytes out of the packfile. -Peff
Re: [PATCH] range-diff: add a --no-patch option to show a summary
Eric Sunshine writes: >> Calling this --[no-]patch might make it harder to integrate it to >> format-patch later, though. I suspect that people would expect >> "format-patch --no-patch ..." to omit both the patch part of the >> range-diff output *AND* the patch that should be applied to the >> codebase (it of course would defeat the point of format-patch, so >> today's format-patch would not pay attention to --no-patch, of >> course). We need to be careful not to break that when it happens. > > Same concern on my side, which is why I was thinking of other, less > confusing, names, such as --summarize or such, though even that is too > general against the full set of git-format-patch options. It could, > perhaps be a separate option, say, "git format-patch > --range-changes=" or something, which would embed the equivalent > of "git range-diff --no-patch ..." in the cover letter. I actually am perfectly fine with --no-patch. My "concern" was merely that we should be careful to make sure that we do not stop producing patches when we plug this patch to format-patch when "format-patch --no-patch" is given; rather, it should only suppress the patch part of range-diff shown in the cover letter.
Re: [PATCH] range-diff: add a --no-patch option to show a summary
On Mon, Nov 5, 2018 at 11:17 PM Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason writes:
> > This change doesn't update git-format-patch with a --no-patch
> > option. That can be added later similar to how format-patch first
> > learned --range-diff, and then --creation-factor in
> > 8631bf1cdd ("format-patch: add --creation-factor tweak for
> > --range-diff", 2018-07-22). I don't see why anyone would want this for
> > format-patch, it pretty much defeats the point of range-diff.
>
> Does it defeats the point of range-diff to omit the patch part in
> the context of the cover letter? How?
>
> I think the output with this option is a good addition to the cover
> letter as an abbreviated form (as opposed to the full range-diff,
> whose support was added earlier) that gives an overview.
I had the same response when reading the commit message but didn't
vocalize it. I could see people wanting to suppress the 'patch' part
of the embedded range-diff in a cover letter (though probably not as
commentary in a single-patch).
> Calling this --[no-]patch might make it harder to integrate it to
> format-patch later, though. I suspect that people would expect
> "format-patch --no-patch ..." to omit both the patch part of the
> range-diff output *AND* the patch that should be applied to the
> codebase (it of course would defeat the point of format-patch, so
> today's format-patch would not pay attention to --no-patch, of
> course). We need to be careful not to break that when it happens.
Same concern on my side, which is why I was thinking of other, less
confusing, names, such as --summarize or such, though even that is too
general against the full set of git-format-patch options. It could,
perhaps be a separate option, say, "git format-patch
--range-changes=" or something, which would embed the equivalent
of "git range-diff --no-patch ..." in the cover letter.
Re: [PATCH v3 00/14] Reduce #ifdef NO_PTHREADS
Nguyễn Thái Ngọc Duy writes: > Changes since v2 > > - more cleanups in grep.c, read-cache.c and index-pack.c > - the send-pack.c changes are back, but this time I just add > async_with_fork() to move NO_PTHREADS back in run-command.c The patches all looked sensible; I'll wait for a few more days to see if anybody else spots something. Thanks.
Re: [PATCH] doc: fix typos in release notes
org...@gmail.com writes: > From: Orgad Shaneh > > Signed-off-by: Orgad Shaneh > --- > Documentation/RelNotes/2.20.0.txt | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Thanks. > > diff --git a/Documentation/RelNotes/2.20.0.txt > b/Documentation/RelNotes/2.20.0.txt > index 4b546d025f..bc0f4e8237 100644 > --- a/Documentation/RelNotes/2.20.0.txt > +++ b/Documentation/RelNotes/2.20.0.txt > @@ -56,7 +56,7 @@ UI, Workflows & Features > > * "git format-patch" learned new "--interdiff" and "--range-diff" > options to explain the difference between this version and the > - previous attempt in the cover letter (or after the tree-dashes as > + previous attempt in the cover letter (or after the three-dashes as > a comment). > > * "git mailinfo" used in "git am" learned to make a best-effort > @@ -78,7 +78,7 @@ UI, Workflows & Features > meaningfully large repository. The users will now see progress > output. > > - * The minimum version of Windows supported by Windows port fo Git is > + * The minimum version of Windows supported by Windows port of Git is > now set to Vista. > > * The completion script (in contrib/) learned to complete a handful of
Re: [PATCH] range-diff: add a --no-patch option to show a summary
Ævar Arnfjörð Bjarmason writes:
> This change doesn't update git-format-patch with a --no-patch
> option. That can be added later similar to how format-patch first
> learned --range-diff, and then --creation-factor in
> 8631bf1cdd ("format-patch: add --creation-factor tweak for
> --range-diff", 2018-07-22). I don't see why anyone would want this for
> format-patch, it pretty much defeats the point of range-diff.
I am OK not to have this option integrated to format-patch from day
one, but I do not think it is a good idea to hint that it should not
be done later.
Does it defeats the point of range-diff to omit the patch part in
the context of the cover letter? How?
I think the output with this option is a good addition to the cover
letter as an abbreviated form (as opposed to the full range-diff,
whose support was added earlier) that gives an overview.
Calling this --[no-]patch might make it harder to integrate it to
format-patch later, though. I suspect that people would expect
"format-patch --no-patch ..." to omit both the patch part of the
range-diff output *AND* the patch that should be applied to the
codebase (it of course would defeat the point of format-patch, so
today's format-patch would not pay attention to --no-patch, of
course). We need to be careful not to break that when it happens.
Re: [PATCH v2 15/16] fsck: reduce word legos to help i18n
Nguyễn Thái Ngọc Duy writes:
> static const char *describe_object(struct object *obj)
> {
> - static struct strbuf buf = STRBUF_INIT;
> - char *name = name_objects ?
> - lookup_decoration(fsck_walk_options.object_names, obj) : NULL;
> + static struct strbuf bufs[4] = {
> + STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
> + };
If you need to repeat _INIT anyway, perhaps you want to actively
omit the 4 from above, no? If you typed 6 by mistake instead, you'd
be in trouble when using the last two elements.
> static int objerror(struct object *obj, const char *err)
> {
> errors_found |= ERROR_OBJECT;
> - objreport(obj, "error", err);
> + fprintf_ln(stderr, "error in %s %s: %s",
> +printable_type(obj), describe_object(obj), err);
> return -1;
> }
Makes sense.
> static int fsck_error_func(struct fsck_options *o,
> struct object *obj, int type, const char *message)
> {
> - objreport(obj, (type == FSCK_WARN) ? "warning" : "error", message);
> - return (type == FSCK_WARN) ? 0 : 1;
> + if (type == FSCK_WARN) {
> + fprintf_ln(stderr, "warning in %s %s: %s",
> +printable_type(obj), describe_object(obj), message);
> + return 0;
> + }
> +
> + fprintf_ln(stderr, "error in %s %s: %s",
> +printable_type(obj), describe_object(obj), message);
> + return 1;
Make it look more symmetrical like the original, perhaps by
if (type == FSCK_WARN) {
...
return 0;
} else { /* FSCK_ERROR */
...
return 1;
}
Actually, wouldn't it be clearer to see what is going on, if we did
it like this instead?
const char *fmt = (type == FSCK_WARN)
? N_("warning in %s %s: %s")
: N_("error in %s %s: %s");
fprintf_ln(stderr, _(fmt),
printable_type(obj), describe_object(obj), message);
return (type == FSCK_WARN) ? 0 : 1;
It would show that in either case we show these three things in the
message. I dunno.
Re: [PATCH v2 13/16] parse-options.c: turn some die() to BUG()
Nguyễn Thái Ngọc Duy writes:
> diff --git a/parse-options.c b/parse-options.c
> index 0bf817193d..3f5f985c1e 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -197,7 +197,7 @@ static int get_value(struct parse_opt_ctx_t *p,
> return 0;
>
> default:
> - die("should not happen, someone must be hit on the forehead");
> + BUG("opt->type %d should not happen", opt->type);
> }
> }
OK, this should not happen.
> @@ -424,7 +424,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
> ctx->flags = flags;
> if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
> (flags & PARSE_OPT_STOP_AT_NON_OPTION))
> - die("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
> + BUG("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
The correctness of this conversion was not immediately obvious, as
"git rev-parse --parse-options" could allow end-users to specify
flags, and when these two flags came together from that codepath, it
is an end-user error that should be diagnosed with die(), not BUG().
It turns out that stop-at-non-option can be passed, but keep-unknown
is not (yet) available to the codepath, so this conversion to BUG()
is correct---an end user futzing with Git correctly compiled from a
bug-free source should not be able to trigger this.
Re: [PATCH v2 12/16] parse-options: replace opterror() with optname()
Nguyễn Thái Ngọc Duy writes:
> There are a few issues with opterror()
>
> - it tries to assemble an English sentence from pieces. This is not
> great for translators because we give them pieces instead of a full
> sentence.
>
> - It's a wrapper around error() and needs some hack to let the
> compiler know it always returns -1.
>
> - Since it takes a string instead of printf format, one call site has
> to assemble the string manually before passing to it.
>
> Kill it and produce the option name with optname(). The user will use
> error() directly. This solves the second and third problems.
The proposed log message is not very friendly to reviewers, as there
is no hint what optname() does nor where it came from; it turns out
that this patch introduces it.
Introduce optname() that does the early half of original
opterror() to come up with the name of the option reported back
to the user, and use it to kill opterror(). The callers of
opterror() now directly call error() using the string returned
by opterror() instead.
or something like that perhaps.
Theoretically not very friendly to topics in flight, but I do not
expect there would be any right now that wants to add new callers of
opterror().
I do agree with the reasoning behind this change. Thanks for
working on it.
>
> It kind helps the first problem as well because "%s does foo" does
> give a translator a full sentence in a sense and let them reorder if
> needed. But it has limitations, if the subject part has to change
> based on the rest of the sentence, that language is screwed. This is
> also why I try to avoid calling optname() when 'flags' is known in
> advance.
>
> Mark of these strings for translation as well while at there.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> builtin/merge.c | 2 +-
> builtin/revert.c| 3 ++-
> parse-options-cb.c | 7 ---
> parse-options.c | 46 +
> parse-options.h | 5 +
> ref-filter.c| 8 +---
> t/t4211-line-log.sh | 2 +-
> 7 files changed, 40 insertions(+), 33 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 92ba7e1c6d..82248d43c3 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -128,7 +128,7 @@ static int option_read_message(struct parse_opt_ctx_t
> *ctx,
> ctx->argc--;
> arg = *++ctx->argv;
> } else
> - return opterror(opt, "requires a value", 0);
> + return error(_("option `%s' requires a value"), opt->long_name);
>
> if (buf->len)
> strbuf_addch(buf, '\n');
> diff --git a/builtin/revert.c b/builtin/revert.c
> index c93393c89b..11190d2ab4 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -69,7 +69,8 @@ static int option_parse_m(const struct option *opt,
>
> replay->mainline = strtol(arg, , 10);
> if (*end || replay->mainline <= 0)
> - return opterror(opt, "expects a number greater than zero", 0);
> + return error(_("option `%s' expects a number greater than
> zero"),
> + opt->long_name);
>
> return 0;
> }
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index e8236534ac..813eb6301b 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -18,7 +18,8 @@ int parse_opt_abbrev_cb(const struct option *opt, const
> char *arg, int unset)
> } else {
> v = strtol(arg, (char **), 10);
> if (*arg)
> - return opterror(opt, "expects a numerical value", 0);
> + return error(_("option `%s' expects a numerical value"),
> + opt->long_name);
> if (v && v < MINIMUM_ABBREV)
> v = MINIMUM_ABBREV;
> else if (v > 40)
> @@ -54,8 +55,8 @@ int parse_opt_color_flag_cb(const struct option *opt, const
> char *arg,
> arg = unset ? "never" : (const char *)opt->defval;
> value = git_config_colorbool(NULL, arg);
> if (value < 0)
> - return opterror(opt,
> - "expects \"always\", \"auto\", or \"never\"", 0);
> + return error(_("option `%s' expects \"always\", \"auto\", or
> \"never\""),
> + opt->long_name);
> *(int *)opt->value = value;
> return 0;
> }
> diff --git a/parse-options.c b/parse-options.c
> index 3b874a83a0..0bf817193d 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -32,7 +32,7 @@ static int get_arg(struct parse_opt_ctx_t *p, const struct
> option *opt,
> p->argc--;
> *arg = *++p->argv;
> } else
> - return opterror(opt, "requires a value", flags);
> + return error(_("%s requires a value"), optname(opt, flags));
> return 0;
> }
>
> @@ -49,7 +49,6 @@ static int opt_command_mode_error(const struct option *opt,
> int flags)
>
Re: Understanding pack format
On Fri, Nov 2, 2018 at 12:00 PM Duy Nguyen wrote: > > On Fri, Nov 2, 2018 at 7:19 AM Junio C Hamano wrote: > > > > Farhan Khan writes: > > > > > ...Where is this in the git code? That might > > > serve as a good guide. > > > > There are two major codepaths. One is used at runtime, giving us > > random access into the packfile with the help with .idx file. The > > other is used when receiving a new packstream to create an .idx > > file. > > The third path is copying/reusing objects in > builtin/pack-objects.c::write_reuse_object(). Since it's mostly > encoding the header of new objects in pack, it could also be a good > starting point. Then you can move to write_no_reuse_object() and get > how the data is encoded, deltified or not (yeah not parsed, but I > think it's more or less the same thing conceptually). > -- > Duy Hi all, To follow-up from the other day, I have been reading the code that retrieves the pack entry for the past 3 days now without much success. But there are quite a few abstractions and I get lost half-way down the line. I am trying to identify where the content from a pack comes from. I traced it back to sha1-file.c:read_object(), which will return the 'content'. I want to know where the 'content' comes from, which seems to come from sha1-file.c:oid_object_info_extended. This goes into packfile.c:find_pack_entry(), but from here I get lost. I do not understand what is happening. How does it retrieve the pack content? I am lost here. Please assist. This is in the technical git documentation, but it was not clear. Thank you, -- Farhan Khan PGP Fingerprint: B28D 2726 E2BC A97E 3854 5ABE 9A9F 00BC D525 16EE
Re: [PATCH v2 09/16] remote.c: turn some error() or die() to BUG()
Nguyễn Thái Ngọc Duy writes:
> The first error, "internal error", is clearly a BUG(). The second two
> are meant to catch calls with invalid parameters and should never
> happen outside the test suite.
Sounds as if it would happen inside test suites.
I guess by "inside the test suite" you mean a test that links broken
callers to this code as if it is a piece of library function, but
"should never happen, even with invalid or malformed end-user
request" would convey the point better, as the normal part of
testing that is done by driing "git we just built from the command
line should not hit these.
The changes all look sensible. Thanks.
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> remote.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 81f4f01b00..257630ff21 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -620,7 +620,7 @@ static void handle_duplicate(struct ref *ref1, struct ref
> *ref2)
>* FETCH_HEAD_IGNORE entries always appear at
>* the end of the list.
>*/
> - die(_("Internal error"));
> + BUG("Internal error");
> }
> }
> free(ref2->peer_ref);
> @@ -707,7 +707,7 @@ static void query_refspecs_multiple(struct refspec *rs,
> int find_src = !query->src;
>
> if (find_src && !query->dst)
> - error("query_refspecs_multiple: need either src or dst");
> + BUG("query_refspecs_multiple: need either src or dst");
>
> for (i = 0; i < rs->nr; i++) {
> struct refspec_item *refspec = >items[i];
> @@ -735,7 +735,7 @@ int query_refspecs(struct refspec *rs, struct
> refspec_item *query)
> char **result = find_src ? >src : >dst;
>
> if (find_src && !query->dst)
> - return error("query_refspecs: need either src or dst");
> + BUG("query_refspecs: need either src or dst");
>
> for (i = 0; i < rs->nr; i++) {
> struct refspec_item *refspec = >items[i];
Re: [PATCH v2 08/16] reflog: mark strings for translation
Nguyễn Thái Ngọc Duy writes:
> if (argc - i < 1)
> - return error("Nothing to delete?");
> + return error(_("no reflog specified to delete"));
Better. Thanks.
Re: [PATCH v2 07/16] read-cache.c: add missing colon separators
Nguyễn Thái Ngọc Duy writes:
> typechange_fmt and added_fmt should have a colon before "needs
> update". Align the statements to make it easier to read and see. Also
> drop the unnecessary ().
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> read-cache.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
Sensible. Thanks.
> diff --git a/read-cache.c b/read-cache.c
> index 858befe738..8d99ae376c 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1492,11 +1492,11 @@ int refresh_index(struct index_state *istate,
> unsigned int flags,
> istate->cache_nr);
>
> trace_performance_enter();
> - modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
> - deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
> - typechange_fmt = (in_porcelain ? "T\t%s\n" : "%s needs update\n");
> - added_fmt = (in_porcelain ? "A\t%s\n" : "%s needs update\n");
> - unmerged_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
> + modified_fmt = in_porcelain ? "M\t%s\n" : "%s: needs update\n";
> + deleted_fmt= in_porcelain ? "D\t%s\n" : "%s: needs update\n";
> + typechange_fmt = in_porcelain ? "T\t%s\n" : "%s: needs update\n";
> + added_fmt = in_porcelain ? "A\t%s\n" : "%s: needs update\n";
> + unmerged_fmt = in_porcelain ? "U\t%s\n" : "%s: needs merge\n";
> for (i = 0; i < istate->cache_nr; i++) {
> struct cache_entry *ce, *new_entry;
> int cache_errno = 0;
Re: [PATCH v2 05/16] read-cache.c: turn die("internal error") to BUG()
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> read-cache.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Makes sense; thanks.
>
> diff --git a/read-cache.c b/read-cache.c
> index d57958233e..0c37f4885e 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -316,7 +316,7 @@ static int ce_match_stat_basic(const struct cache_entry
> *ce, struct stat *st)
> changed |= DATA_CHANGED;
> return changed;
> default:
> - die("internal error: ce_mode is %o", ce->ce_mode);
> + BUG("unsupported ce_mode: %o", ce->ce_mode);
> }
>
> changed |= match_stat_data(>ce_stat_data, st);
> @@ -2356,14 +2356,14 @@ void validate_cache_entries(const struct index_state
> *istate)
>
> for (i = 0; i < istate->cache_nr; i++) {
> if (!istate) {
> - die("internal error: cache entry is not allocated from
> expected memory pool");
> + BUG("cache entry is not allocated from expected memory
> pool");
> } else if (!istate->ce_mem_pool ||
> !mem_pool_contains(istate->ce_mem_pool,
> istate->cache[i])) {
> if (!istate->split_index ||
> !istate->split_index->base ||
> !istate->split_index->base->ce_mem_pool ||
>
> !mem_pool_contains(istate->split_index->base->ce_mem_pool, istate->cache[i]))
> {
> - die("internal error: cache entry is not
> allocated from expected memory pool");
> + BUG("cache entry is not allocated from expected
> memory pool");
> }
> }
> }
Re: [PATCH v2 03/16] archive.c: mark more strings for translation
Nguyễn Thái Ngọc Duy writes:
> Two messages also print extra information to be more useful
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> archive.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/archive.c b/archive.c
> index 9d16b7fadf..d8f6e1ce30 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -385,12 +385,12 @@ static void parse_treeish_arg(const char **argv,
> int refnamelen = colon - name;
>
> if (!dwim_ref(name, refnamelen, , ))
> - die("no such ref: %.*s", refnamelen, name);
> + die(_("no such ref: %.*s"), refnamelen, name);
> free(ref);
> }
>
> if (get_oid(name, ))
> - die("Not a valid object name");
> + die(_("not a valid object name: %s"), name);
Much better than the previous one that gave the name upfront.
>
> commit = lookup_commit_reference_gently(ar_args->repo, , 1);
> if (commit) {
> @@ -403,7 +403,7 @@ static void parse_treeish_arg(const char **argv,
>
> tree = parse_tree_indirect();
> if (tree == NULL)
> - die("not a tree object");
> + die(_("not a tree object: %s"), oid_to_hex());
Likewise; as oid_to_hex() would be quite long compared to the rest
of the message, this is a vast improvement from the previous round.
> if (prefix) {
> struct object_id tree_oid;
> @@ -413,7 +413,7 @@ static void parse_treeish_arg(const char **argv,
> err = get_tree_entry(>object.oid, prefix, _oid,
>);
> if (err || !S_ISDIR(mode))
> - die("current working directory is untracked");
> + die(_("current working directory is untracked"));
>
> tree = parse_tree_indirect(_oid);
> }
Re: [PATCH v2 01/16] git.c: mark more strings for translation
Nguyễn Thái Ngọc Duy writes:
> One string is slightly updated to keep consistency with the rest:
> die() should with lowercase.
s/should/& begin/, I think, in which case I could locally touch up.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> git.c | 30 +++---
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/git.c b/git.c
> index adac132956..5fd30da093 100644
> --- a/git.c
> +++ b/git.c
> @@ -338,27 +338,27 @@ static int handle_alias(int *argcp, const char ***argv)
> if (ret >= 0) /* normal exit */
> exit(ret);
>
> - die_errno("while expanding alias '%s': '%s'",
> - alias_command, alias_string + 1);
> + die_errno(_("while expanding alias '%s': '%s'"),
> + alias_command, alias_string + 1);
> }
> count = split_cmdline(alias_string, _argv);
> if (count < 0)
> - die("Bad alias.%s string: %s", alias_command,
> + die(_("bad alias.%s string: %s"), alias_command,
> split_cmdline_strerror(count));
> option_count = handle_options(_argv, , );
> if (envchanged)
> - die("alias '%s' changes environment variables.\n"
> - "You can use '!git' in the alias to do this",
> - alias_command);
> + die(_("alias '%s' changes environment variables.\n"
> + "You can use '!git' in the alias to do this"),
> + alias_command);
> memmove(new_argv - option_count, new_argv,
> count * sizeof(char *));
> new_argv -= option_count;
>
> if (count < 1)
> - die("empty alias for %s", alias_command);
> + die(_("empty alias for %s"), alias_command);
>
> if (!strcmp(alias_command, new_argv[0]))
> - die("recursive alias: %s", alias_command);
> + die(_("recursive alias: %s"), alias_command);
>
> trace_argv_printf(new_argv,
> "trace: alias expansion: %s =>",
> @@ -409,7 +409,7 @@ static int run_builtin(struct cmd_struct *p, int argc,
> const char **argv)
>
> if (!help && get_super_prefix()) {
> if (!(p->option & SUPPORT_SUPER_PREFIX))
> - die("%s doesn't support --super-prefix", p->cmd);
> + die(_("%s doesn't support --super-prefix"), p->cmd);
> }
>
> if (!help && p->option & NEED_WORK_TREE)
> @@ -433,11 +433,11 @@ static int run_builtin(struct cmd_struct *p, int argc,
> const char **argv)
>
> /* Check for ENOSPC and EIO errors.. */
> if (fflush(stdout))
> - die_errno("write failure on standard output");
> + die_errno(_("write failure on standard output"));
> if (ferror(stdout))
> - die("unknown write failure on standard output");
> + die(_("unknown write failure on standard output"));
> if (fclose(stdout))
> - die_errno("close failed on standard output");
> + die_errno(_("close failed on standard output"));
> return 0;
> }
>
> @@ -648,7 +648,7 @@ static void execv_dashed_external(const char **argv)
> int status;
>
> if (get_super_prefix())
> - die("%s doesn't support --super-prefix", argv[0]);
> + die(_("%s doesn't support --super-prefix"), argv[0]);
>
> if (use_pager == -1 && !is_builtin(argv[0]))
> use_pager = check_pager_config(argv[0]);
> @@ -760,7 +760,7 @@ int cmd_main(int argc, const char **argv)
> if (skip_prefix(cmd, "git-", )) {
> argv[0] = cmd;
> handle_builtin(argc, argv);
> - die("cannot handle %s as a builtin", cmd);
> + die(_("cannot handle %s as a builtin"), cmd);
> }
>
> /* Look for flags.. */
> @@ -773,7 +773,7 @@ int cmd_main(int argc, const char **argv)
> } else {
> /* The user didn't specify a command; give them help */
> commit_pager_choice();
> - printf("usage: %s\n\n", git_usage_string);
> + printf(_("usage: %s\n\n"), git_usage_string);
> list_common_cmds_help();
> printf("\n%s\n", _(git_more_info_string));
> exit(1);
Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers)
Anders Waldenborg writes: > AFAICU strbuf_expand doesn't suffer from the worst things that printf(3) > suffers from wrt untrusted format string (i.e no printf style %n which > can write to memory, and no vaargs on stack which allows leaking random > stuff). > > The separator option is part of the full format string. If a malicious > user can specify that, they can't really do anything new, as the > separator only can expand %n and %xNN, which they already can do in the > full string. > > But maybe I'm missing something? I just wanted to make sure somebody thought it through (and hoped that that somebody might be you). I do not offhand see a readily usable exploit vector myself.
Re: [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function
Anders Waldenborg writes: > Junio C Hamano writes: >> I do not think "fundamental" is the best name for this, but I agree >> that it would be useful to split the helpers into one that is >> "constant across commits" and the other one that is "per commit". > > Any suggestions for a better name? > > standalone? simple? invariant? free? If these are like %n for LF or %09 for HT, perhaps they are constants or "literals".
Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines
Duy Nguyen writes: > I think the intent of writing "This reverts " to encourage > writing "because ..." is good, but in practice many people just simply > not do it. And by not describing anything at all (footers don't count) > some commit hook can force people to actually write something. > > But for the transition period I think we need to keep both anyway, I do not see any need to qualify the statement with "for the transition period". You showed *no* need to transition, but I do agree that adding a fixed-spelled footer in addition to what we produce in the body is a good idea. When we know a feature with good intent is not used properly by many people, the first thing to do is not talk about removing it, but to think how we can educate people to make good use of it. And if we know a feature with good intent is not used by many people but we know that "many" is not "100%", why are we talking about removing it at all? > Yep. I'll code something up to print both by default with config knobs > to disable either. Unless you have some better plan of course. Does it make sense to put both, with exactly the same piece of information? I am not sure whom it would help. The tools need to be updated to deal with both old and new format if the pick-origin information is used, instead of getting updated to learn new and forget old format, as existing commits in their history do not know about the new format and their tools need to understand them. I'd say it would be sufficient to have a per-repository knob that says which one to use, and between the release we add that knob and the release we make the new format the default, when we do not see the knob is set to either way, keep warning that in the future the default will change and give advice that the knob can be used to either keep the old format or update to the new format before or after the default switch (in addition to squelch the warning and the advice).
Re: Design of multiple hash support
On Mon, Nov 05, 2018 at 02:00:42PM -0800, Jonathan Nieder wrote: > Hi, > > Duy Nguyen wrote: > > On Mon, Nov 5, 2018 at 2:02 AM brian m. carlson > > wrote: > > >> There are basically two approaches I can take. The first is to provide > >> each command that needs to learn about this with its own --hash > >> argument. So we'd have: > >> > >> git init --hash=sha256 > >> git show-index --hash=sha256 >> > >> The other alternative is that we provide a global option to git, which > >> is parsed by all programs, like so: > >> > >> git --hash=sha256 init > >> git --hash=sha256 show-index [...] > > I'm leaning towards "git foo --hash=". > > Can you say a little more about the semantics of the option? For > commands like "git init", I tend to agree with Duy here, since it > allows each command's manual to describe what the option means in the > context of that command. Sure. The semantics for git init are "produce a repository with this hash algorithm". The semantics for git index-pack are "the pack I want you to index uses this hash algorithm". Essentially, more generically, the semantics are "the repository or data object uses this hash algorithm". > For "git show-index", ideally Git should use the object format named > in the idx file. I agree that will be the eventual goal. It will also be what I ship in the final series, in all likelihood. I have most of pack v3 implemented, but it's not complete yet. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: Design of multiple hash support
On Mon, Nov 05, 2018 at 10:03:21AM -0800, Stefan Beller wrote: > On Sun, Nov 4, 2018 at 6:36 PM Junio C Hamano wrote: > > > > "brian m. carlson" writes: > > > > > I'm currently working on getting Git to support multiple hash algorithms > > > in the same binary (SHA-1 and SHA-256). In order to have a fully > > > functional binary, we'll need to have some way of indicating to certain > > > commands (such as init and show-index) that they should assume a certain > > > hash algorithm. > > > > > > There are basically two approaches I can take. The first is to provide > > > each command that needs to learn about this with its own --hash > > > argument. So we'd have: > > > > > > git init --hash=sha256 > > > git show-index --hash=sha256 > > > > > The other alternative is that we provide a global option to git, which > > > is parsed by all programs, like so: > > > > > > git --hash=sha256 init > > > git --hash=sha256 show-index > > > I am assuming that "show-index" above is a typo for something like > > "hash-object"? > Actually both seem plausible, as both do not require > RUN_SETUP, which means they cannot rely on the > extensions.objectFormat setting. Correct. In general, I assume that options that want a repository will use the repository for that information. There are a small number of programs, such as init, that need to either set up a repository (without reference to another repository) or need to inspect files without necessarily being in a repository. For example, we will want to have a way of indicating which hash we would like to use in a fresh repository. I am for the moment assuming that we're in a stage 4 configuration: that is, that we're all SHA-1 or all SHA-256. A clone will provide this for us, but a git init will not. Also, our pack index v3 format knows about which hash algorithm is in use, but packs are not labeled with the algorithm they use. This isn't really a problem in normal use, since we always know from context which algorithm is in use, but we'll need to indicate to index-pack (which technically need not run in a repository) which algorithm it should use. show-index will eventually learn to parse the index itself to learn which algorithms are in use, so it is technically not required here. > When having a global setting, would that override the configured > object format extension in a repository, or do we error out? > > So maybe > > git -c extensions.objectFormat=sha256 init > > is the way to go, for now? (Are repository format extensions parsed > just like normal config, such that non-RUN_SETUP commands > can rely on the (non-)existence to determine whether to use > the default or the given hash function?) The extensions callbacks are only handled in check_repo_format, so they necessarily require a repository. This is not new with my code. Furthermore, one would have to specify "-c core.repositoryformatversion=1" as well, as extensions require that version in order to have any effect. My current approach for the testsuite is to have git init honor a new GIT_DEFAULT_HASH environment variable so we need not modify every place in the testsuite that calls git init (of which there are many). That may or may not be greeted with joy by reviewers, but it seemed to be the minimum viable approach. > There is a section "Object names on the command line" > in Documentation/technical/hash-function-transition.txt > and I assume that this before the "dark launch" > phase, so I would expect the latter to work (no error > but conversion/translation on the fly) eventually as a goal. > But the former might be in scope of one series. Currently, I'm not implementing the stage 1-3 implementations. I'm merely going from the point where we have a binary that does only SHA-256 and cannot perform SHA-1 operations at all to a stage 4 implementation, where the binary can do either, but a repository is wholly one or the other. > > It can work this way: > > > > - read HEAD, discover that I am on 'master' branch, read refs/heads/master > >to learn the object name in 40-hex, realize that it cannot be > >sha256 and report "corrupt ref". > > > > Or it can work this way: > > > > - read repository format, realize it is a good old sha1 repository. > > > > - do the usual thing to get to read_object() to read the commit > >object data for the commit at HEAD, doing all of it in sha1. > > > > - in the commit object data, locate references to other objects > >that use sha1 name. > > > > - replace these sha1 references with their sha256 counterparts and > >show the result. > > > > I am guessing that you are doing the former as a good first step, in > > which case, as an option that changes/affects the behaviour of git > > globally, I think "git --hash=sha256" would make sense, like other > > global options like --literal-pathspecs and --no-replace-objects. Right now, we always read the repository configuration when possible, and honor that. I'm not planning,
Re: Git Slowness on Windows w/o Internet
Good point. I'll check it out. Thanks for the tip. Thanks, Peter Thanks, Peter Kostyukov Senior Systems Engineer Kohl's Department Stores - KIC Office: 262-703-6533 On Sat, Nov 3, 2018 at 6:48 PM Philip Oakley wrote: > > > On 03/11/2018 16:44, brian m. carlson wrote: > > On Fri, Nov 02, 2018 at 11:10:51AM -0500, Peter Kostyukov wrote: > >> Wanted to bring to your attention an issue that we discovered on our > >> Windows Jenkins nodes with git scm installed (git.exe). Our Jenkins > >> servers don't have Internet access. It appears that git.exe is trying > >> to connect to various Cloudflare and Akamai CDN instances over the > >> Internet when it first runs and it keeps trying to connect to these > >> CDNs every git.exe execution until it makes a successful attempt. See > >> the screenshot attached with the details. > >> > >> Enabling Internet access via proxy fixes the issue and git.exe > >> continues to work fast on the next attempts to run git.exe > >> > >> Is there any configuration setting that can disable this git's > >> behavior or is there any other workaround without allowing Internet > >> access? Otherwise, every git command run on a server without the > >> Internet takes about 30 seconds to complete. > > > > Git itself doesn't make any attempt to access those systems unless it's > > configured to do so (e.g. a remote is set up to talk to those systems > > and fetch or pull is used). > > > > It's possible that you're using a distribution package that performs > > this behavior, say, to check for updates. I'd recommend that you > > contact the distributor, which in this case might be Git for Windows, > > and see if they can tell you more about what's going on. The URL for > > that project is at https://github.com/git-for-windows/git. > > > > The normal Git for Windows install includes an option to check for > updates at a suitable rate. Maybe you are hitting that. It can be > switched off. > > -- > Philip CONFIDENTIALITY NOTICE: This is a transmission from Kohl's Department Stores, Inc. and may contain information which is confidential and proprietary. If you are not the addressee, any disclosure, copying or distribution or use of the contents of this message is expressly prohibited. If you have received this transmission in error, please destroy it and notify us immediately at 262-703-7000. CAUTION: Internet and e-mail communications are Kohl's property and Kohl's reserves the right to retrieve and read any message created, sent and received. Kohl's reserves the right to monitor messages by authorized Kohl's Associates at any time without any further consent.
Re: Failed stash caused untracked changes to be lost
On 11/05, Quinn, David wrote:
> Hi,
>
> Thanks for the reply. Sorry I forgot the version number, completely
> slipped my mind. At the time of writing the report it was Git ~ 2.17
> I believe. All of our software is updated centrally at my work, we
> have received an update since writing this to 2.19.1. Unfortunately
> because of it being centrally controlled, I couldn't update and try
> the latest version at the time (and now I can't go back and check
> exactly what version I had).
>
> I've never even looked at the git source or contributing before so I
> wouldn't be sure where to start. If you (or someone) is happy to
> point me in the right direction I'd be happy to take a look, I can't
> promise I'll be able to get anything done in a timely manner (or at
> all)
Sure I'd be happy to help :) There's a nice document in the
git-for-windows repository [*1*] that gives a good introduction into
developing git. Some of the advise is applicable to both Windows and
linux, but it should be especially helpful for you as you seem to be
working in a windows environment.
The stash implementation is currently written in shell script, and
lives in 'git-stash.sh'. There is currently an effort under way of
re-writing this in C, but as we don't know when that's going to be
merged yet, it's probably worth fixing this in the shell script for
now.
Don't worry about making any promises, or getting it done very soon.
This is no longer a data loss bug at this time, so it's not critical
to fix it immediately, but it should definitely be fixed at some
point.
Some of us also hang out on the #git-devel IRC channel on freenode,
which can be a good place to ask questions.
[*1*]: https://github.com/git-for-windows/git/blob/master/CONTRIBUTING.md
> Thanks
>
> -Original Message-
> From: Thomas Gummerer
> Sent: 03 November 2018 15:35
> To: Quinn, David
> Cc: git@vger.kernel.org
> Subject: Re: Failed stash caused untracked changes to be lost
>
> Exercise Caution: This email is from an external source.
>
>
> On 10/23, Quinn, David wrote:
> >
> > Issue: While running a git stash command including the '-u' flag to include
> > untracked files, the command failed due to arguments in the incorrect
> > order. After this untracked files the were present had been removed and
> > permanently lost.
>
> Thanks for your report (and sorry for the late reply)!
>
> I believe this (somewhat) fixed in 833622a945 ("stash push: avoid printing
> errors", 2018-03-19), which was first included in Git 2.18.
> Your message doesn't state which version of Git you encountered the bug, but
> I'm going to assume with something below 2.18 (For future reference, please
> include the version of Git in bug reports, or even better test with the
> latest version of Git, as the bug may have been fixed in the meantime).
>
> Now I'm saying somewhat fixed above, because we still create an stash if a
> pathspec that doesn't match any files is passed to the command, but then
> don't remove anything from the working tree, which is a bit confusing.
>
> I think the right solution here would be to error out early if we were given
> a pathspec that doesn't match anything. I'll look into that, unless you're
> interested in giving it a try? :)
>
> > Environment: Windows 10, Powershell w/ PoshGit
> >
> >
> > State before running command: 9 Modified files, 2 (new) untracked
> > files
> >
> > Note: I only wanted to commit some of the modified files (essentially
> > all the files/changes I wanted to commit were in one directory)
> >
> > Actual command run: git stash push -u -- Directory/To/Files/* -m "My
> > Message"
> >
> > Returned:
> >
> > Saved working directory and index state WIP on [BranchName]: [Commit
> > hash] [Commit Message]
> > fatal: pathspec '-m' did not match any files
> > error: unrecognized input
> >
> > State after Command ran: 9 Modifed files, 0 untracked files
> >
> >
> > The command I should have ran should have been
> >
> > git stash push -u -m "My Message"? -- Directory/To/Files/*
> >
> >
> > I have found the stash that was created by running this command:
> >
> > gitk --all $(git fsck --no-reflog | Select-String "(dangling
> > commit )(.*)" | %{ $_.Line.Split(' ')[2] }) ?
> > and searching for the commit number that was returned from the original
> > (paritally failed??) stash command. However there is nothing in that stash.
> > It is empty.
> >
> >
> >
> > I think that the fact my untracked files were lost is not correct
> > behaviour and hence why I'm filing this bug report
> >
> >
> >
> >
> >
> > NOTICE: This message, and any attachments, are for the intended
> > recipient(s) only, may contain information that is privileged, confidential
> > and/or proprietary and subject to important terms and conditions available
> > at E-Communication
> > Disclaimer.
> > If you are
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Hi Hannes, On Mon, 5 Nov 2018, Johannes Sixt wrote: > Am 05.11.18 um 00:26 schrieb Junio C Hamano: > > OK, thanks. It seems that the relative silence after this message is > > a sign that the resulting patch after squashing is what everybody is > > happey with? > > I'm not 100% happy. I'll resend a squashed patch, but it has to wait as > I have to catch a train now. Thank you for running with this. Ciao, Dscho
Re: Design of multiple hash support
Hi, Duy Nguyen wrote: > On Mon, Nov 5, 2018 at 2:02 AM brian m. carlson > wrote: >> There are basically two approaches I can take. The first is to provide >> each command that needs to learn about this with its own --hash >> argument. So we'd have: >> >> git init --hash=sha256 >> git show-index --hash=sha256 > >> The other alternative is that we provide a global option to git, which >> is parsed by all programs, like so: >> >> git --hash=sha256 init >> git --hash=sha256 show-index I'm leaning towards "git foo --hash=". Can you say a little more about the semantics of the option? For commands like "git init", I tend to agree with Duy here, since it allows each command's manual to describe what the option means in the context of that command. For "git show-index", ideally Git should use the object format named in the idx file. >> There's also the question of what we want to call the option. The >> obvious name is --hash, which is intuitive and straightforward. >> However, the transition plan names the config option >> extensions.objectFormat, [...] > --object-format is less vague than --hash. The downside is it's longer > (more to type) but I'm counting on git-completion.bash and the guess > that people rarely need to use this option. Agreed. --object-format makes more sense to me than --hash, since it's more precise about what the option affects. Thanks for looking into this. Sincerely, Jonathan
Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines
Hi,
On Sun, 4 Nov 2018, Duy Nguyen wrote:
> On Sun, Nov 4, 2018 at 5:45 PM Phillip Wood wrote:
> >
> > On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote:
> > > When a commit is reverted (or cherry-picked with -x) we add an English
> > > sentence recording that commit id in the new commit message. Make
> > > these real trailer lines instead so that they are more friendly to
> > > parsers (especially "git interpret-trailers").
> > >
> > > A reverted commit will have a new trailer
> > >
> > > Revert:
> > >
> > > Similarly a cherry-picked commit with -x will have
> > >
> > > Cherry-Pick:
> >
> > I think this is a good idea though I wonder if it will break someones
> > script that is looking for the messages generated by -x at the moment.
>
> It will [1] but I still think it's worth the trouble. The script will
> be less likely to break after, and you can use git-interpret-trailers
> instead of plain grep.
Since this is a wilfull backwards-incompatible patch, it needs to be done
carefully.
I am not enthused by the idea of breaking power users' setups, and I could
imagine that a much better way to do it would be to introduce a config
setting that guards the new behavior, then add a deprecation notice when
-x is used without that config setting, and then let that simmer for a
couple of versions.
Ciao,
Johannes
>
> [1]
> https://public-inbox.org/git/20181017143921.gr270...@devbig004.ftw2.facebook.com/
>
> > > @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command
> > > command, struct commit *commit,
> > > base_label = msg.label;
> > > next = parent;
> > > next_label = msg.parent_label;
> > > - strbuf_addstr(, "Revert \"");
> > > - strbuf_addstr(, msg.subject);
> > > - strbuf_addstr(, "\"\n\nThis reverts commit ");
> > > - strbuf_addstr(, oid_to_hex(>object.oid));
> > > -
> > > - if (commit->parents && commit->parents->next) {
> > > - strbuf_addstr(, ", reversing\nchanges made
> > > to ");
> > > - strbuf_addstr(,
> > > oid_to_hex(>object.oid));
> > > - }
> >
> > As revert currently records the parent given on the command line when
> > reverting a merge commit it would probably be a good idea to add that
> > either as a separate trailer or to the Revert: trailer and possibly also
> > generate it for cherry picks.
>
> My mistake. I didn't read carefully and thought it was logging
> commit->parents, which is pointless.
>
> So what should be the trailer for this (I don't think putting it in
> Revert: is a good idea, too much to parse)? Revert-parent: ?
> Revert-merge: ?
>
> > > - strbuf_addstr(, ".\n");
> > > + strbuf_addf(, "Revert \"%s\"\n\n", msg.subject);
> >
> > If the message already contains trailers then should we just append the
> > Revert trailer those rather than inserting "\n\n"?
>
> Umm.. but this \n\n is for separating the subject and the body. I
> think we need it anyway, trailer or not.
>
> > > @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command
> > > command, struct commit *commit,
> > > strbuf_complete_line();
> > > if (!has_conforming_footer(, NULL, 0))
> > > strbuf_addch(, '\n');
> > > - strbuf_addstr(, cherry_picked_prefix);
> > > - strbuf_addstr(,
> > > oid_to_hex(>object.oid));
> > > - strbuf_addstr(, ")\n");
> > > + strbuf_addf(, "Cherry-Pick: %s\n",
> > > + oid_to_hex(>object.oid));
>
> I will probably make this "Cherry-picked-from:" to match our S-o-b style.
> --
> Duy
>
Re: [RFC v1] Add virtual file system settings and hook proc
Hi Ævar, On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > The only potential downside I see is that there's currently exactly one > implementation of this sort of thing in the wild, so we risk any such > API becoming too tied up with just what GVFS wants, and not what we'd > like to support with such a thing in general. This is what e.g. the w3c > tries to avoid with having multiple browser implementations before > something is standardized. It is my understanding that Ben is quite interested in ideas how to make this *not* tied up with VFSforGit. I'd think that he would welcome any good ideas you have in that direction. Ciao, Dscho
Re: [PATCH] range-diff: add a --no-patch option to show a summary
On Mon, Nov 05 2018, Eric Sunshine wrote: > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason > wrote: >> Add a --no-patch option which shows which changes got removed, added >> or moved etc., without showing the diff associated with them. > > This option existed in the very first version[1] of range-diff (then > called branch-diff) implemented by Dscho, although it was called > --no-patches (with an "es"), which it inherited from tbdiff. I think > someone (possibly me) pointed out that --no-patch (sans "es") would be > more consistent with existing Git options. I don't recall why Dscho > removed the option during the re-rolls, but the explanation may be in > that thread. Thanks for digging. Big thread, not going to re-read it now. I'd just like to have this. > I was also wondering if --summarize or --summary-only might be a > better name, describing the behavior at a higher level, but since > there is precedent for --no-patch (or --no-patches in tbdiff), perhaps > the name is fine as is. I think we should aim to keep a 1=1 mapping between range-diff and log/show options when possible, even though the output might have a slightly different flavor as my 4th paragraph discussing a potential --stat talks about. E.g. I can imagine that range-diff --no-patch --stat --summary would not show the patch, but a stat as described there, plus e.g. a "create mode..." if applicable. This change implements only a tiny fraction of that, but it would be very neat if we supported more stuff, and showed it in range-diff-y way, e.g. some compact format showing: 1 file changed, 3->2 insertions(+), 10->9 deletions(-) create mode 100(6 -> 7)44 new-executable > The patch itself looks okay. > > [1]: > https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schinde...@gmx.de/
Wildcard URL config matching
In a272b9e70a ("urlmatch: allow globbing for the URL host part",
2017-01-31), we added support for wildcard matching for URLs when
reading from .git/config. Now it's possible to specify an option like
http.http://*.example.com/.cookieFile and have it match for the URL
http://foo.example.com. However, since this option also allows
wildcards at any level, the following also matches:
http.http://*.*.*/.cookieFile.
I'm wondering if it was intentional to allow this behavior or if we
intended to allow only the leftmost label (or labels) to be a wildcard.
The tests seem to test only the leftmost label, and that is the behavior
that one has for TLS certificates, for example. I don't really see a
situation where one would want to match hostname labels in an arbitrary
position, but perhaps I'm simply not being imaginative enough in
thinking through the use cases.
Regardless of what we decide, I'll be sending a patch, either to add
additional tests, or correct the code (or both).
I ask because we're implementing this behavior for Git LFS, where we
don't iterate over all configuration keys, instead looking up certain
values in a hash. We'll need to make some changes in order to have
things work correctly if we want to implement the current Git behavior
to prevent combinatorial explosion.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Am 05.11.18 um 08:01 schrieb Johannes Sixt: Am 05.11.18 um 00:26 schrieb Junio C Hamano: OK, thanks. It seems that the relative silence after this message is a sign that the resulting patch after squashing is what everybody is happey with? I'm not 100% happy. I see the patch is already in next. Never mind. The patch text is fine, I just wanted to modify the commit message a bit. Thanks, -- Hannes
Re: [RFC v1] Add virtual file system settings and hook proc
On 11/4/2018 4:01 PM, brian m. carlson wrote: On Sun, Nov 04, 2018 at 07:34:01AM +0100, Duy Nguyen wrote: On Wed, Oct 31, 2018 at 9:53 PM Ben Peart wrote: It's more than a dynamic sparse-checkout because the same list is also used to exclude any file/folder not listed. That means any file not listed won't ever be updated by git (like in 'checkout' for example) so 'stale' files could be left in the working directory. It also means git won't find new/untracked files unless they are specifically added to the list. OK. I'm not at all interested in carrying maintenance burden for some software behind closed doors. I could see values in having a more flexible sparse checkout but this now seems like very tightly designed for GVFS. So unless there's another use case (preferably open source) for this, I don't think this should be added in git.git. I should point out that VFS for Git is an open-source project and will likely have larger use than just at Microsoft. There are both Windows and Mac clients and there are plans for a Linux client as well. Ideally, it would work with an unmodified upstream Git, which is (I assume) why Ben is sending this series. Personally, I don't love the current name used in this series. I don't see this patch as introducing a virtual file system in the Unix sense of that word, and I think calling it that in Git core will be confusing to Unix users. I would prefer to see it as a hook (maybe called "sparse-checkout" or "sparse-exclude"; better names are okay), and simply turn it on based on whether or not there's an appropriate hook file there and whether core.sparseCheckout is on (or possibly with hook.sparseExclude or something). With a design more like that, I don't see a problem with it in principle. I'm really bad at naming so am happy to choose something else that will be more descriptive to the community at large. The name came from the fact that we started with the (equally awful) 'VFS for Git' and this was the big enabling feature in git so for better or worse it got saddled with the same 'VFS' name. In other feedback it was suggested to not add a core.vfs setting that was the path to the hook and I like that. I can change it to core.sparseExclude (unless someone has something better) and hard code the hook name for now. I do like the idea of having config based hooks so when I get some time I will put together a patch series to implement that.
Re: [PATCH] range-diff: add a --no-patch option to show a summary
On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason wrote: > Add a --no-patch option which shows which changes got removed, added > or moved etc., without showing the diff associated with them. This option existed in the very first version[1] of range-diff (then called branch-diff) implemented by Dscho, although it was called --no-patches (with an "es"), which it inherited from tbdiff. I think someone (possibly me) pointed out that --no-patch (sans "es") would be more consistent with existing Git options. I don't recall why Dscho removed the option during the re-rolls, but the explanation may be in that thread. I was also wondering if --summarize or --summary-only might be a better name, describing the behavior at a higher level, but since there is precedent for --no-patch (or --no-patches in tbdiff), perhaps the name is fine as is. The patch itself looks okay. [1]: https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schinde...@gmx.de/
Re: [RFC v1] Add virtual file system settings and hook proc
On 11/5/2018 10:22 AM, Duy Nguyen wrote: On Sun, Nov 4, 2018 at 10:01 PM brian m. carlson wrote: On Sun, Nov 04, 2018 at 07:34:01AM +0100, Duy Nguyen wrote: On Wed, Oct 31, 2018 at 9:53 PM Ben Peart wrote: It's more than a dynamic sparse-checkout because the same list is also used to exclude any file/folder not listed. That means any file not listed won't ever be updated by git (like in 'checkout' for example) so 'stale' files could be left in the working directory. It also means git won't find new/untracked files unless they are specifically added to the list. OK. I'm not at all interested in carrying maintenance burden for some software behind closed doors. I could see values in having a more flexible sparse checkout but this now seems like very tightly designed for GVFS. So unless there's another use case (preferably open source) for this, I don't think this should be added in git.git. I should point out that VFS for Git is an open-source project and will likely have larger use than just at Microsoft. There are both Windows and Mac clients and there are plans for a Linux client as well. Ideally, it would work with an unmodified upstream Git, which is (I assume) why Ben is sending this series. Ah I didn't know that. Thank you. I'll have to look at this GVFS some time then. If we're going to support GVFS though, I think there should be a big (RFC perhaps) series that includes everything to at least give an overview what the end game looks like. Then it could be split up into smaller series. We've always had the goal of not needing a fork at all and are continually working to bring the list of differences to zero but in the mean time, you can see the entire set of changes we've made here [1]. If you look, most of them are changes we are already in process of submitting to git (ie midx, tracing, etc) or patches we fast tracked from master to our branch (your unpack_trees() optimizations for example). Most of the others are small tweaks and features for performance or to smooth integration issues. This RFC contains the core changes that were required to enable VFS for Git. [1] https://github.com/git-for-windows/git/compare/master...Microsoft:gvfs-2.19.1
Re: [RFC v1] Add virtual file system settings and hook proc
On 11/5/2018 10:26 AM, Duy Nguyen wrote: On Mon, Nov 5, 2018 at 12:40 PM Ævar Arnfjörð Bjarmason wrote: On Sun, Nov 04 2018, Duy Nguyen wrote: On Wed, Oct 31, 2018 at 9:53 PM Ben Peart wrote: +core.virtualFilesystem:: + If set, the value of this variable is used as a command which + will identify all files and directories that are present in + the working directory. Git will only track and update files + listed in the virtual file system. Using the virtual file system + will supersede the sparse-checkout settings which will be ignored. + See the "virtual file system" section of linkgit:githooks[6]. It sounds like "virtual file system" is just one of the use cases for this feature, which is more about a dynamic source of sparse-checkout bits. Perhaps name the config key with something along sparse checkout instead of naming it after a use case. It's more than a dynamic sparse-checkout because the same list is also used to exclude any file/folder not listed. That means any file not listed won't ever be updated by git (like in 'checkout' for example) so 'stale' files could be left in the working directory. It also means git won't find new/untracked files unless they are specifically added to the list. OK. I'm not at all interested in carrying maintenance burden for some software behind closed doors. I could see values in having a more flexible sparse checkout but this now seems like very tightly designed for GVFS. So unless there's another use case (preferably open source) for this, I don't think this should be added in git.git. I haven't looked at the patch in any detail beyond skimming it, and you're more familiar with this area... But in principle I'm very interested in getting something like this in git.git, even if we were to assume GVFS was a 100% proprietary implementation. I have nothing against building a GVFS-like solution. If what's submitted can be the building blocks for that, great! But if it was just for GVFS (and it was not available to everybody) then no thank you. Not only is VFS for Git open source and is/will be supported on Windows, Mac and Linux, the interface being proposed is quite generic so should be usable for other implementations. To use it, you just need to provide a hook that will return a list of files git should pay attention to (using a subset of the existing sparse-checkout format). If you see anything that would make using it difficult for other solutions to use, let's fix it now!
[PATCH] range-diff: add a --no-patch option to show a summary
Add a --no-patch option which shows which changes got removed, added
or moved etc., without showing the diff associated with them.
This allows for using range-diff as a poor man's "shortlog" for
force-pushed branches to see what changed without getting into the
details of what specifically. E.g. diffing the latest forced-push to
"pu" gives us:
$ ./git-range-diff --no-patch b58974365b...711aaa392f | head -n 10
-: -- > 1: b613de67c4 diff: differentiate error handling in
parse_color_moved_ws
28: c731affab0 ! 2: 23c4bbe28e build: link with curl-defined linker flags
-: -- > 3: 14f74d5907 git-worktree.txt: correct linkgit command
name
-: -- > 4: 29d51e214c sequencer.c: remove a stray semicolon
-: -- > 5: b7845cebc0 tree-walk.c: fix overoptimistic inclusion
in :(exclude) matching
-: -- > 6: 1a550529b1 t/t7510-signed-commit.sh: Add %GP to
custom format checks
-: -- > 7: 1e690847d1 t/t7510-signed-commit.sh: add signing
subkey to Eris Discordia key
9: d13ecb7d81 ! 8: d8ad847421 Add a base implementation of SHA-256
support
10: 3f0382eef8 = 9: cdae1d391c sha256: add an SHA-256 implementation
using libgcrypt
11: 2422fd4227 = 10: 7d81aa0857 hash: add an SHA-256 implementation using
OpenSSL
That would print a total of 44 lines of output, but the full
range-diff output with --patch is 460 lines.
I thought of implementing --stat too. It would be neat if passing
DIFF_FORMAT_DIFFSTAT just worked, but using that shows the underlying
implementation details of how range-diff works, instead of a useful
diffstat. So I'll leave that to a future change. Such a feature should
be something like a textual summary of the --patch output itself,
e.g.:
N hunks, X insertions(+), Y deletions(-)
This change doesn't update git-format-patch with a --no-patch
option. That can be added later similar to how format-patch first
learned --range-diff, and then --creation-factor in
8631bf1cdd ("format-patch: add --creation-factor tweak for
--range-diff", 2018-07-22). I don't see why anyone would want this for
format-patch, it pretty much defeats the point of range-diff.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/git-range-diff.txt | 4 +++
builtin/log.c| 2 +-
builtin/range-diff.c | 5 +++-
log-tree.c | 2 +-
range-diff.c | 6 -
range-diff.h | 1 +
t/t3206-range-diff.sh| 45
7 files changed, 61 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
index f693930fdb..d0473cbcb1 100644
--- a/Documentation/git-range-diff.txt
+++ b/Documentation/git-range-diff.txt
@@ -57,6 +57,10 @@ to revert to color all lines according to the outer diff
markers
See the ``Algorithm`` section below for an explanation why this is
needed.
+--no-patch::
+ Don't show the range-diff itself, only which patches are the
+ same or were added or removed etc.
+
::
Compare the commits specified by the two ranges, where
`` is considered an older version of ``.
diff --git a/builtin/log.c b/builtin/log.c
index 061d4fd864..e063bcf2dd 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1096,7 +1096,7 @@ static void make_cover_letter(struct rev_info *rev, int
use_stdout,
if (rev->rdiff1) {
fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
show_range_diff(rev->rdiff1, rev->rdiff2,
- rev->creation_factor, 1, >diffopt);
+ rev->creation_factor, 1, 1, >diffopt);
}
}
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index f01a0be851..70c2761751 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -16,11 +16,14 @@ int cmd_range_diff(int argc, const char **argv, const char
*prefix)
int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
struct diff_options diffopt = { NULL };
int simple_color = -1;
+ int patch = 1;
struct option options[] = {
OPT_INTEGER(0, "creation-factor", _factor,
N_("Percentage by which creation is weighted")),
OPT_BOOL(0, "no-dual-color", _color,
N_("use simple diff colors")),
+ OPT_BOOL('p', "patch", ,
+N_("show patch output")),
OPT_END()
};
int i, j, res = 0;
@@ -92,7 +95,7 @@ int cmd_range_diff(int argc, const char **argv, const char
*prefix)
}
res = show_range_diff(range1.buf, range2.buf, creation_factor,
- simple_color < 1, );
+ simple_color < 1, patch, );
strbuf_release();
strbuf_release();
diff --git a/log-tree.c b/log-tree.c
index
Re: [RFC v1] Add virtual file system settings and hook proc
On 11/4/2018 7:02 PM, Junio C Hamano wrote:
Ben Peart writes:
+ if (*dtype == DT_UNKNOWN)
+ *dtype = get_dtype(NULL, istate, pathname, pathlen);
We try to defer paying cost to determine unknown *dtype as late as
possible by having this call in last_exclude_matching_from_list(),
and not here. If we are doing this, we probably should update the
callpaths that call last_exclude_matching_from_list() to make the
caller responsible for doing get_dtype() and drop the lazy finding
of dtype from the callee. Alternatively, the new "is excluded from
vfs" helper can learn to do the lazy get_dtype() just like the
existing last_exclude_matching_from_list() does. I suspect the
latter may be simpler.
In is_excluded_from_virtualfilesystem() dtype can't be lazy because it
is always needed (which is why I test and die if it isn't known).
You make a call to that function even when virtual-file-system hook
is not in use, i.e. instead of the caller saying
if (is_vfs_in_use()) {
*dtype = get_dtype(...);
if (is_excluded_from_vfs(...) > 0)
return 1;
}
your caller makes an unconditional call to is_excluded_from_vfs().
Isn't that the only reason why you break the laziness of determining
dtype?
You can keep the caller simple by making an unconditional call, but
maintain the laziness by updating the callig convention to pass
dtype (not *dtype) to the function, e.g..
if (is_excluded_from_vfs(pathname, pathlen, dtype) > 0)
return 1;
and then at the beginning of the helper
if (is_vfs_in_use())
return -1; /* undetermined */
*dtype = get_dtype(...);
... whatever logic it has now ...
no?
Oops! You're right, I docall get_dtype() even if the vfs isn't in use.
I'll add an additional test to avoid doing that. Thank you.
I did look into moving the delay load logic into is_excluded_from_vfs()
but get_dtype() is static to dir.c and I'd prefer to keep it that way if
possible.
Your comments are all feedback on the code - how it was implemented,
style, etc. Any thoughts on whether this is something we could/should
merge into master (after any necessary cleanup)? Would anyone else
find this interesting/helpful?
I am pretty much neutral. Not strongly opposed to it, but not all
that interested until seeing its integration with the "userland" to
see how the whole thing works ;-)
[PATCH] doc: fix typos in release notes
From: Orgad Shaneh Signed-off-by: Orgad Shaneh --- Documentation/RelNotes/2.20.0.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/RelNotes/2.20.0.txt b/Documentation/RelNotes/2.20.0.txt index 4b546d025f..bc0f4e8237 100644 --- a/Documentation/RelNotes/2.20.0.txt +++ b/Documentation/RelNotes/2.20.0.txt @@ -56,7 +56,7 @@ UI, Workflows & Features * "git format-patch" learned new "--interdiff" and "--range-diff" options to explain the difference between this version and the - previous attempt in the cover letter (or after the tree-dashes as + previous attempt in the cover letter (or after the three-dashes as a comment). * "git mailinfo" used in "git am" learned to make a best-effort @@ -78,7 +78,7 @@ UI, Workflows & Features meaningfully large repository. The users will now see progress output. - * The minimum version of Windows supported by Windows port fo Git is + * The minimum version of Windows supported by Windows port of Git is now set to Vista. * The completion script (in contrib/) learned to complete a handful of -- 2.19.1
Re: [PATCH v1] refresh_index: remove unnecessary calls to preload_index()
On Mon, Nov 5, 2018 at 8:30 PM Ben Peart wrote: > > From: Ben Peart > > With refresh_index() learning to utilize preload_index() to speed up its > operation there is no longer any benefit to having the caller preload the > index first. Remove those unneeded calls by calling read_index() instead of > the preload variant. > > There is no measurable performance impact of this patch - the 2nd call to > preload_index() bails out quickly but there is no reason to call it twice. Obviously correct. It's not shown in the context lines, but there's also a refresh_index() after read_index() in sequencer.c too. -- Duy
[PATCH v1] refresh_index: remove unnecessary calls to preload_index()
From: Ben Peart
With refresh_index() learning to utilize preload_index() to speed up its
operation there is no longer any benefit to having the caller preload the
index first. Remove those unneeded calls by calling read_index() instead of
the preload variant.
There is no measurable performance impact of this patch - the 2nd call to
preload_index() bails out quickly but there is no reason to call it twice.
Signed-off-by: Ben Peart
---
Notes:
Base Ref:
Web-Diff: https://github.com/benpeart/git/commit/384f7fed53
Checkout: git fetch https://github.com/benpeart/git no-index-preload-v1 &&
git checkout 384f7fed53
builtin/commit.c | 2 +-
builtin/describe.c | 2 +-
builtin/update-index.c | 2 +-
sequencer.c| 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 074bd9a551..96d336ec3d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1363,7 +1363,7 @@ int cmd_status(int argc, const char **argv, const char
*prefix)
if (status_format != STATUS_FORMAT_PORCELAIN &&
status_format != STATUS_FORMAT_PORCELAIN_V2)
progress_flag = REFRESH_PROGRESS;
- read_index_preload(_index, , progress_flag);
+ read_index(_index);
refresh_index(_index,
REFRESH_QUIET|REFRESH_UNMERGED|progress_flag,
, NULL, NULL);
diff --git a/builtin/describe.c b/builtin/describe.c
index c48c34e866..cc118448ee 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -629,7 +629,7 @@ int cmd_describe(int argc, const char **argv, const char
*prefix)
struct argv_array args = ARGV_ARRAY_INIT;
int fd, result;
- read_cache_preload(NULL);
+ read_cache();
refresh_index(_index,
REFRESH_QUIET|REFRESH_UNMERGED,
NULL, NULL, NULL);
fd = hold_locked_index(_lock, 0);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 07c10bcb7d..0e1dcf0438 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -782,7 +782,7 @@ struct refresh_params {
static int refresh(struct refresh_params *o, unsigned int flag)
{
setup_work_tree();
- read_cache_preload(NULL);
+ read_cache();
*o->has_errors |= refresh_cache(o->flags | flag);
return 0;
}
diff --git a/sequencer.c b/sequencer.c
index 9e1ab3a2a7..ab2048ac3a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1919,7 +1919,7 @@ static int read_and_refresh_cache(struct replay_opts
*opts)
{
struct lock_file index_lock = LOCK_INIT;
int index_fd = hold_locked_index(_lock, 0);
- if (read_index_preload(_index, NULL, 0) < 0) {
+ if (read_index(_index) < 0) {
rollback_lock_file(_lock);
return error(_("git %s: failed to read the index"),
_(action_name(opts)));
base-commit: 095c8dc8c2a9d61783dbae79a7f6e8d80092696f
--
2.18.0.windows.1
[PATCH v2 13/16] parse-options.c: turn some die() to BUG()
These two strings are clearly not for the user to see. Reduce the
violence in one string while at there.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
parse-options.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 0bf817193d..3f5f985c1e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -197,7 +197,7 @@ static int get_value(struct parse_opt_ctx_t *p,
return 0;
default:
- die("should not happen, someone must be hit on the forehead");
+ BUG("opt->type %d should not happen", opt->type);
}
}
@@ -424,7 +424,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
ctx->flags = flags;
if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
(flags & PARSE_OPT_STOP_AT_NON_OPTION))
- die("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
+ BUG("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
parse_options_check(options);
}
--
2.19.1.1005.gac84295441
[PATCH v2 16/16] fsck: mark strings for translation
Two die() are updated to start with lowercase to be consistent with
the rest.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/fsck.c | 106 -
t/t1410-reflog.sh | 6 +--
t/t1450-fsck.sh| 50 -
t/t6050-replace.sh | 4 +-
t/t7415-submodule-names.sh | 6 +--
5 files changed, 90 insertions(+), 82 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 504f47d7a4..0720708977 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -85,7 +85,7 @@ static const char *printable_type(struct object *obj)
ret = type_name(obj->type);
if (!ret)
- ret = "unknown";
+ ret = _("unknown");
return ret;
}
@@ -116,7 +116,8 @@ static int fsck_config(const char *var, const char *value,
void *cb)
static int objerror(struct object *obj, const char *err)
{
errors_found |= ERROR_OBJECT;
- fprintf_ln(stderr, "error in %s %s: %s",
+ /* TRANSLATORS: e.g. error in tree 01bfda: */
+ fprintf_ln(stderr, _("error in %s %s: %s"),
printable_type(obj), describe_object(obj), err);
return -1;
}
@@ -125,12 +126,14 @@ static int fsck_error_func(struct fsck_options *o,
struct object *obj, int type, const char *message)
{
if (type == FSCK_WARN) {
- fprintf_ln(stderr, "warning in %s %s: %s",
+ /* TRANSLATORS: e.g. warning in tree 01bfda:
*/
+ fprintf_ln(stderr, _("warning in %s %s: %s"),
printable_type(obj), describe_object(obj), message);
return 0;
}
- fprintf_ln(stderr, "error in %s %s: %s",
+ /* TRANSLATORS: e.g. error in tree 01bfda: */
+ fprintf_ln(stderr, _("error in %s %s: %s"),
printable_type(obj), describe_object(obj), message);
return 1;
}
@@ -148,17 +151,18 @@ static int mark_object(struct object *obj, int type, void
*data, struct fsck_opt
*/
if (!obj) {
/* ... these references to parent->fld are safe here */
- printf("broken link from %7s %s\n",
- printable_type(parent), describe_object(parent));
- printf("broken link from %7s %s\n",
- (type == OBJ_ANY ? "unknown" : type_name(type)),
"unknown");
+ printf_ln(_("broken link from %7s %s"),
+ printable_type(parent), describe_object(parent));
+ printf_ln(_("broken link from %7s %s"),
+ (type == OBJ_ANY ? _("unknown") : type_name(type)),
+ _("unknown"));
errors_found |= ERROR_REACHABLE;
return 1;
}
if (type != OBJ_ANY && obj->type != type)
/* ... and the reference to parent is safe here */
- objerror(parent, "wrong object type in link");
+ objerror(parent, _("wrong object type in link"));
if (obj->flags & REACHABLE)
return 0;
@@ -174,8 +178,8 @@ static int mark_object(struct object *obj, int type, void
*data, struct fsck_opt
if (!(obj->flags & HAS_OBJ)) {
if (parent && !has_object_file(>oid)) {
- printf_ln("broken link from %7s %s\n"
- " to %7s %s",
+ printf_ln(_("broken link from %7s %s\n"
+ " to %7s %s"),
printable_type(parent),
describe_object(parent),
printable_type(obj),
@@ -243,8 +247,8 @@ static void check_reachable_object(struct object *obj)
return;
if (has_object_pack(>oid))
return; /* it is in pack - forget about it */
- printf("missing %s %s\n", printable_type(obj),
- describe_object(obj));
+ printf_ln(_("missing %s %s"), printable_type(obj),
+ describe_object(obj));
errors_found |= ERROR_REACHABLE;
return;
}
@@ -269,8 +273,8 @@ static void check_unreachable_object(struct object *obj)
* since this is something that is prunable.
*/
if (show_unreachable) {
- printf("unreachable %s %s\n", printable_type(obj),
- describe_object(obj));
+ printf_ln(_("unreachable %s %s"), printable_type(obj),
+ describe_object(obj));
return;
}
@@ -288,8 +292,8 @@ static void check_unreachable_object(struct object *obj)
*/
if (!(obj->flags & USED)) {
if (show_dangling)
- printf("dangling %s %s\n", printable_type(obj),
- describe_object(obj));
+
[PATCH v2 12/16] parse-options: replace opterror() with optname()
There are a few issues with opterror()
- it tries to assemble an English sentence from pieces. This is not
great for translators because we give them pieces instead of a full
sentence.
- It's a wrapper around error() and needs some hack to let the
compiler know it always returns -1.
- Since it takes a string instead of printf format, one call site has
to assemble the string manually before passing to it.
Kill it and produce the option name with optname(). The user will use
error() directly. This solves the second and third problems.
It kind helps the first problem as well because "%s does foo" does
give a translator a full sentence in a sense and let them reorder if
needed. But it has limitations, if the subject part has to change
based on the rest of the sentence, that language is screwed. This is
also why I try to avoid calling optname() when 'flags' is known in
advance.
Mark of these strings for translation as well while at there.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/merge.c | 2 +-
builtin/revert.c| 3 ++-
parse-options-cb.c | 7 ---
parse-options.c | 46 +
parse-options.h | 5 +
ref-filter.c| 8 +---
t/t4211-line-log.sh | 2 +-
7 files changed, 40 insertions(+), 33 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 92ba7e1c6d..82248d43c3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -128,7 +128,7 @@ static int option_read_message(struct parse_opt_ctx_t *ctx,
ctx->argc--;
arg = *++ctx->argv;
} else
- return opterror(opt, "requires a value", 0);
+ return error(_("option `%s' requires a value"), opt->long_name);
if (buf->len)
strbuf_addch(buf, '\n');
diff --git a/builtin/revert.c b/builtin/revert.c
index c93393c89b..11190d2ab4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -69,7 +69,8 @@ static int option_parse_m(const struct option *opt,
replay->mainline = strtol(arg, , 10);
if (*end || replay->mainline <= 0)
- return opterror(opt, "expects a number greater than zero", 0);
+ return error(_("option `%s' expects a number greater than
zero"),
+opt->long_name);
return 0;
}
diff --git a/parse-options-cb.c b/parse-options-cb.c
index e8236534ac..813eb6301b 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -18,7 +18,8 @@ int parse_opt_abbrev_cb(const struct option *opt, const char
*arg, int unset)
} else {
v = strtol(arg, (char **), 10);
if (*arg)
- return opterror(opt, "expects a numerical value", 0);
+ return error(_("option `%s' expects a numerical value"),
+opt->long_name);
if (v && v < MINIMUM_ABBREV)
v = MINIMUM_ABBREV;
else if (v > 40)
@@ -54,8 +55,8 @@ int parse_opt_color_flag_cb(const struct option *opt, const
char *arg,
arg = unset ? "never" : (const char *)opt->defval;
value = git_config_colorbool(NULL, arg);
if (value < 0)
- return opterror(opt,
- "expects \"always\", \"auto\", or \"never\"", 0);
+ return error(_("option `%s' expects \"always\", \"auto\", or
\"never\""),
+opt->long_name);
*(int *)opt->value = value;
return 0;
}
diff --git a/parse-options.c b/parse-options.c
index 3b874a83a0..0bf817193d 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -32,7 +32,7 @@ static int get_arg(struct parse_opt_ctx_t *p, const struct
option *opt,
p->argc--;
*arg = *++p->argv;
} else
- return opterror(opt, "requires a value", flags);
+ return error(_("%s requires a value"), optname(opt, flags));
return 0;
}
@@ -49,7 +49,6 @@ static int opt_command_mode_error(const struct option *opt,
int flags)
{
const struct option *that;
- struct strbuf message = STRBUF_INIT;
struct strbuf that_name = STRBUF_INIT;
/*
@@ -67,13 +66,13 @@ static int opt_command_mode_error(const struct option *opt,
strbuf_addf(_name, "--%s", that->long_name);
else
strbuf_addf(_name, "-%c", that->short_name);
- strbuf_addf(, ": incompatible with %s", that_name.buf);
+ error(_("%s is incompatible with %s"),
+ optname(opt, flags), that_name.buf);
strbuf_release(_name);
- opterror(opt, message.buf, flags);
- strbuf_release();
return -1;
}
- return opterror(opt, ": incompatible with something else", flags);
+ return error(_("%s : incompatible with something else"),
+
[PATCH v2 15/16] fsck: reduce word legos to help i18n
These messages will be marked for translation later. Reduce word legos
and give translators almost full phrases. describe_object() is updated
so that it can be called from printf() twice.
While at there, remove \n from the strings to reduce a bit of work
from translators.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/fsck.c | 62 ++
1 file changed, 37 insertions(+), 25 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 06eb421720..504f47d7a4 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -52,16 +52,24 @@ static int name_objects;
static const char *describe_object(struct object *obj)
{
- static struct strbuf buf = STRBUF_INIT;
- char *name = name_objects ?
- lookup_decoration(fsck_walk_options.object_names, obj) : NULL;
+ static struct strbuf bufs[4] = {
+ STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
+ };
+ static int b = 0;
+ struct strbuf *buf;
+ char *name = NULL;
- strbuf_reset();
- strbuf_addstr(, oid_to_hex(>oid));
+ if (name_objects)
+ name = lookup_decoration(fsck_walk_options.object_names, obj);
+
+ buf = bufs + b;
+ b = (b + 1) % ARRAY_SIZE(bufs);
+ strbuf_reset(buf);
+ strbuf_addstr(buf, oid_to_hex(>oid));
if (name)
- strbuf_addf(, " (%s)", name);
+ strbuf_addf(buf, " (%s)", name);
- return buf.buf;
+ return buf->buf;
}
static const char *printable_type(struct object *obj)
@@ -105,25 +113,26 @@ static int fsck_config(const char *var, const char
*value, void *cb)
return git_default_config(var, value, cb);
}
-static void objreport(struct object *obj, const char *msg_type,
- const char *err)
-{
- fprintf(stderr, "%s in %s %s: %s\n",
- msg_type, printable_type(obj), describe_object(obj), err);
-}
-
static int objerror(struct object *obj, const char *err)
{
errors_found |= ERROR_OBJECT;
- objreport(obj, "error", err);
+ fprintf_ln(stderr, "error in %s %s: %s",
+ printable_type(obj), describe_object(obj), err);
return -1;
}
static int fsck_error_func(struct fsck_options *o,
struct object *obj, int type, const char *message)
{
- objreport(obj, (type == FSCK_WARN) ? "warning" : "error", message);
- return (type == FSCK_WARN) ? 0 : 1;
+ if (type == FSCK_WARN) {
+ fprintf_ln(stderr, "warning in %s %s: %s",
+ printable_type(obj), describe_object(obj), message);
+ return 0;
+ }
+
+ fprintf_ln(stderr, "error in %s %s: %s",
+ printable_type(obj), describe_object(obj), message);
+ return 1;
}
static struct object_array pending;
@@ -165,10 +174,12 @@ static int mark_object(struct object *obj, int type, void
*data, struct fsck_opt
if (!(obj->flags & HAS_OBJ)) {
if (parent && !has_object_file(>oid)) {
- printf("broken link from %7s %s\n",
-printable_type(parent),
describe_object(parent));
- printf(" to %7s %s\n",
-printable_type(obj), describe_object(obj));
+ printf_ln("broken link from %7s %s\n"
+ " to %7s %s",
+ printable_type(parent),
+ describe_object(parent),
+ printable_type(obj),
+ describe_object(obj));
errors_found |= ERROR_REACHABLE;
}
return 1;
@@ -371,10 +382,11 @@ static int fsck_obj(struct object *obj, void *buffer,
unsigned long size)
struct tag *tag = (struct tag *) obj;
if (show_tags && tag->tagged) {
- printf("tagged %s %s", printable_type(tag->tagged),
- describe_object(tag->tagged));
- printf(" (%s) in %s\n", tag->tag,
- describe_object(>object));
+ printf_ln("tagged %s %s (%s) in %s",
+ printable_type(tag->tagged),
+ describe_object(tag->tagged),
+ tag->tag,
+ describe_object(>object));
}
}
--
2.19.1.1005.gac84295441
[PATCH v2 14/16] parse-options.c: mark more strings for translation
One error is updated to start with lowercase to be consistent with the
rest.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
parse-options.c | 14 +++---
t/t0040-parse-options.sh | 4 ++--
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 3f5f985c1e..86aad2e185 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -319,8 +319,8 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const
char *arg,
}
if (ambiguous_option) {
- error("Ambiguous option: %s "
- "(could be --%s%s or --%s%s)",
+ error(_("ambiguous option: %s "
+ "(could be --%s%s or --%s%s)"),
arg,
(ambiguous_flags & OPT_UNSET) ? "no-" : "",
ambiguous_option->long_name,
@@ -353,7 +353,7 @@ static void check_typos(const char *arg, const struct
option *options)
return;
if (starts_with(arg, "no-")) {
- error ("did you mean `--%s` (with two dashes ?)", arg);
+ error(_("did you mean `--%s` (with two dashes ?)"), arg);
exit(129);
}
@@ -361,7 +361,7 @@ static void check_typos(const char *arg, const struct
option *options)
if (!options->long_name)
continue;
if (starts_with(options->long_name, arg)) {
- error ("did you mean `--%s` (with two dashes ?)", arg);
+ error(_("did you mean `--%s` (with two dashes ?)"),
arg);
exit(129);
}
}
@@ -644,11 +644,11 @@ int parse_options(int argc, const char **argv, const char
*prefix,
break;
default: /* PARSE_OPT_UNKNOWN */
if (ctx.argv[0][1] == '-') {
- error("unknown option `%s'", ctx.argv[0] + 2);
+ error(_("unknown option `%s'"), ctx.argv[0] + 2);
} else if (isascii(*ctx.opt)) {
- error("unknown switch `%c'", *ctx.opt);
+ error(_("unknown switch `%c'"), *ctx.opt);
} else {
- error("unknown non-ascii option in string: `%s'",
+ error(_("unknown non-ascii option in string: `%s'"),
ctx.argv[0]);
}
usage_with_options(usagestr, options);
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 17d0c18feb..e46b1e02f0 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -228,7 +228,7 @@ EOF
test_expect_success 'detect possible typos' '
test_must_fail test-tool parse-options -boolean >output 2>output.err &&
test_must_be_empty output &&
- test_cmp typo.err output.err
+ test_i18ncmp typo.err output.err
'
cat >typo.err <<\EOF
@@ -238,7 +238,7 @@ EOF
test_expect_success 'detect possible typos' '
test_must_fail test-tool parse-options -ambiguous >output 2>output.err
&&
test_must_be_empty output &&
- test_cmp typo.err output.err
+ test_i18ncmp typo.err output.err
'
test_expect_success 'keep some options as arguments' '
--
2.19.1.1005.gac84295441
[PATCH v2 01/16] git.c: mark more strings for translation
One string is slightly updated to keep consistency with the rest:
die() should with lowercase.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
git.c | 30 +++---
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/git.c b/git.c
index adac132956..5fd30da093 100644
--- a/git.c
+++ b/git.c
@@ -338,27 +338,27 @@ static int handle_alias(int *argcp, const char ***argv)
if (ret >= 0) /* normal exit */
exit(ret);
- die_errno("while expanding alias '%s': '%s'",
- alias_command, alias_string + 1);
+ die_errno(_("while expanding alias '%s': '%s'"),
+ alias_command, alias_string + 1);
}
count = split_cmdline(alias_string, _argv);
if (count < 0)
- die("Bad alias.%s string: %s", alias_command,
+ die(_("bad alias.%s string: %s"), alias_command,
split_cmdline_strerror(count));
option_count = handle_options(_argv, , );
if (envchanged)
- die("alias '%s' changes environment variables.\n"
-"You can use '!git' in the alias to do this",
-alias_command);
+ die(_("alias '%s' changes environment variables.\n"
+ "You can use '!git' in the alias to do this"),
+ alias_command);
memmove(new_argv - option_count, new_argv,
count * sizeof(char *));
new_argv -= option_count;
if (count < 1)
- die("empty alias for %s", alias_command);
+ die(_("empty alias for %s"), alias_command);
if (!strcmp(alias_command, new_argv[0]))
- die("recursive alias: %s", alias_command);
+ die(_("recursive alias: %s"), alias_command);
trace_argv_printf(new_argv,
"trace: alias expansion: %s =>",
@@ -409,7 +409,7 @@ static int run_builtin(struct cmd_struct *p, int argc,
const char **argv)
if (!help && get_super_prefix()) {
if (!(p->option & SUPPORT_SUPER_PREFIX))
- die("%s doesn't support --super-prefix", p->cmd);
+ die(_("%s doesn't support --super-prefix"), p->cmd);
}
if (!help && p->option & NEED_WORK_TREE)
@@ -433,11 +433,11 @@ static int run_builtin(struct cmd_struct *p, int argc,
const char **argv)
/* Check for ENOSPC and EIO errors.. */
if (fflush(stdout))
- die_errno("write failure on standard output");
+ die_errno(_("write failure on standard output"));
if (ferror(stdout))
- die("unknown write failure on standard output");
+ die(_("unknown write failure on standard output"));
if (fclose(stdout))
- die_errno("close failed on standard output");
+ die_errno(_("close failed on standard output"));
return 0;
}
@@ -648,7 +648,7 @@ static void execv_dashed_external(const char **argv)
int status;
if (get_super_prefix())
- die("%s doesn't support --super-prefix", argv[0]);
+ die(_("%s doesn't support --super-prefix"), argv[0]);
if (use_pager == -1 && !is_builtin(argv[0]))
use_pager = check_pager_config(argv[0]);
@@ -760,7 +760,7 @@ int cmd_main(int argc, const char **argv)
if (skip_prefix(cmd, "git-", )) {
argv[0] = cmd;
handle_builtin(argc, argv);
- die("cannot handle %s as a builtin", cmd);
+ die(_("cannot handle %s as a builtin"), cmd);
}
/* Look for flags.. */
@@ -773,7 +773,7 @@ int cmd_main(int argc, const char **argv)
} else {
/* The user didn't specify a command; give them help */
commit_pager_choice();
- printf("usage: %s\n\n", git_usage_string);
+ printf(_("usage: %s\n\n"), git_usage_string);
list_common_cmds_help();
printf("\n%s\n", _(git_more_info_string));
exit(1);
--
2.19.1.1005.gac84295441
[PATCH v2 07/16] read-cache.c: add missing colon separators
typechange_fmt and added_fmt should have a colon before "needs
update". Align the statements to make it easier to read and see. Also
drop the unnecessary ().
Signed-off-by: Nguyễn Thái Ngọc Duy
---
read-cache.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 858befe738..8d99ae376c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1492,11 +1492,11 @@ int refresh_index(struct index_state *istate, unsigned
int flags,
istate->cache_nr);
trace_performance_enter();
- modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
- deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
- typechange_fmt = (in_porcelain ? "T\t%s\n" : "%s needs update\n");
- added_fmt = (in_porcelain ? "A\t%s\n" : "%s needs update\n");
- unmerged_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
+ modified_fmt = in_porcelain ? "M\t%s\n" : "%s: needs update\n";
+ deleted_fmt= in_porcelain ? "D\t%s\n" : "%s: needs update\n";
+ typechange_fmt = in_porcelain ? "T\t%s\n" : "%s: needs update\n";
+ added_fmt = in_porcelain ? "A\t%s\n" : "%s: needs update\n";
+ unmerged_fmt = in_porcelain ? "U\t%s\n" : "%s: needs merge\n";
for (i = 0; i < istate->cache_nr; i++) {
struct cache_entry *ce, *new_entry;
int cache_errno = 0;
--
2.19.1.1005.gac84295441
[PATCH v2 08/16] reflog: mark strings for translation
One string "nothing to delete?" is rephrased to be more helpful.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/reflog.c | 34 +++---
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index b5941c1ff3..5a74ccf7ab 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -13,11 +13,15 @@
/* NEEDSWORK: switch to using parse_options */
static const char reflog_expire_usage[] =
-"git reflog expire [--expire=] [--expire-unreachable=] [--rewrite]
[--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all] ...";
+N_("git reflog expire [--expire=] "
+ "[--expire-unreachable=] "
+ "[--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] "
+ "[--verbose] [--all] ...");
static const char reflog_delete_usage[] =
-"git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose]
...";
+N_("git reflog delete [--rewrite] [--updateref] "
+ "[--dry-run | -n] [--verbose] ...");
static const char reflog_exists_usage[] =
-"git reflog exists ";
+N_("git reflog exists ");
static timestamp_t default_reflog_expire;
static timestamp_t default_reflog_expire_unreachable;
@@ -556,7 +560,7 @@ static int cmd_reflog_expire(int argc, const char **argv,
const char *prefix)
break;
}
else if (arg[0] == '-')
- usage(reflog_expire_usage);
+ usage(_(reflog_expire_usage));
else
break;
}
@@ -569,7 +573,7 @@ static int cmd_reflog_expire(int argc, const char **argv,
const char *prefix)
if (cb.cmd.stalefix) {
repo_init_revisions(the_repository, , prefix);
if (flags & EXPIRE_REFLOGS_VERBOSE)
- printf("Marking reachable objects...");
+ printf(_("Marking reachable objects..."));
mark_reachable_objects(, 0, 0, NULL);
if (flags & EXPIRE_REFLOGS_VERBOSE)
putchar('\n');
@@ -598,7 +602,7 @@ static int cmd_reflog_expire(int argc, const char **argv,
const char *prefix)
char *ref;
struct object_id oid;
if (!dwim_log(argv[i], strlen(argv[i]), , )) {
- status |= error("%s points nowhere!", argv[i]);
+ status |= error(_("%s points nowhere!"), argv[i]);
continue;
}
set_reflog_expiry_param(, explicit_expiry, ref);
@@ -644,13 +648,13 @@ static int cmd_reflog_delete(int argc, const char **argv,
const char *prefix)
break;
}
else if (arg[0] == '-')
- usage(reflog_delete_usage);
+ usage(_(reflog_delete_usage));
else
break;
}
if (argc - i < 1)
- return error("Nothing to delete?");
+ return error(_("no reflog specified to delete"));
for ( ; i < argc; i++) {
const char *spec = strstr(argv[i], "@{");
@@ -659,12 +663,12 @@ static int cmd_reflog_delete(int argc, const char **argv,
const char *prefix)
int recno;
if (!spec) {
- status |= error("Not a reflog: %s", argv[i]);
+ status |= error(_("not a reflog: %s"), argv[i]);
continue;
}
if (!dwim_log(argv[i], spec - argv[i], , )) {
- status |= error("no reflog for '%s'", argv[i]);
+ status |= error(_("no reflog for '%s'"), argv[i]);
continue;
}
@@ -699,7 +703,7 @@ static int cmd_reflog_exists(int argc, const char **argv,
const char *prefix)
break;
}
else if (arg[0] == '-')
- usage(reflog_exists_usage);
+ usage(_(reflog_exists_usage));
else
break;
}
@@ -707,10 +711,10 @@ static int cmd_reflog_exists(int argc, const char **argv,
const char *prefix)
start = i;
if (argc - start != 1)
- usage(reflog_exists_usage);
+ usage(_(reflog_exists_usage));
if (check_refname_format(argv[start], REFNAME_ALLOW_ONELEVEL))
- die("invalid ref format: %s", argv[start]);
+ die(_("invalid ref format: %s"), argv[start]);
return !reflog_exists(argv[start]);
}
@@ -719,12 +723,12 @@ static int cmd_reflog_exists(int argc, const char **argv,
const char *prefix)
*/
static const char reflog_usage[] =
-"git reflog [ show | expire | delete | exists ]";
+N_("git reflog [ show | expire | delete | exists ]");
int cmd_reflog(int argc, const char **argv, const char *prefix)
{
if (argc > 1 && !strcmp(argv[1], "-h"))
-
[PATCH v2 09/16] remote.c: turn some error() or die() to BUG()
The first error, "internal error", is clearly a BUG(). The second two
are meant to catch calls with invalid parameters and should never
happen outside the test suite.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
remote.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/remote.c b/remote.c
index 81f4f01b00..257630ff21 100644
--- a/remote.c
+++ b/remote.c
@@ -620,7 +620,7 @@ static void handle_duplicate(struct ref *ref1, struct ref
*ref2)
* FETCH_HEAD_IGNORE entries always appear at
* the end of the list.
*/
- die(_("Internal error"));
+ BUG("Internal error");
}
}
free(ref2->peer_ref);
@@ -707,7 +707,7 @@ static void query_refspecs_multiple(struct refspec *rs,
int find_src = !query->src;
if (find_src && !query->dst)
- error("query_refspecs_multiple: need either src or dst");
+ BUG("query_refspecs_multiple: need either src or dst");
for (i = 0; i < rs->nr; i++) {
struct refspec_item *refspec = >items[i];
@@ -735,7 +735,7 @@ int query_refspecs(struct refspec *rs, struct refspec_item
*query)
char **result = find_src ? >src : >dst;
if (find_src && !query->dst)
- return error("query_refspecs: need either src or dst");
+ BUG("query_refspecs: need either src or dst");
for (i = 0; i < rs->nr; i++) {
struct refspec_item *refspec = >items[i];
--
2.19.1.1005.gac84295441
[PATCH v2 10/16] remote.c: mark messages for translation
The two strings are slightly modified to be consistent with the rest:
die() and error() start with a lowercase.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
remote.c | 43 ++-
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/remote.c b/remote.c
index 257630ff21..11b33d1625 100644
--- a/remote.c
+++ b/remote.c
@@ -359,7 +359,7 @@ static int handle_config(const char *key, const char
*value, void *cb)
return 0;
/* Handle remote..* variables */
if (*name == '/') {
- warning("Config remote shorthand cannot begin with '/': %s",
+ warning(_("config remote shorthand cannot begin with '/': %s"),
name);
return 0;
}
@@ -406,7 +406,7 @@ static int handle_config(const char *key, const char
*value, void *cb)
if (!remote->receivepack)
remote->receivepack = v;
else
- error("more than one receivepack given, using the
first");
+ error(_("more than one receivepack given, using the
first"));
} else if (!strcmp(subkey, "uploadpack")) {
const char *v;
if (git_config_string(, key, value))
@@ -414,7 +414,7 @@ static int handle_config(const char *key, const char
*value, void *cb)
if (!remote->uploadpack)
remote->uploadpack = v;
else
- error("more than one uploadpack given, using the
first");
+ error(_("more than one uploadpack given, using the
first"));
} else if (!strcmp(subkey, "tagopt")) {
if (!strcmp(value, "--no-tags"))
remote->fetch_tags = -1;
@@ -680,7 +680,7 @@ static int match_name_with_pattern(const char *key, const
char *name,
size_t namelen;
int ret;
if (!kstar)
- die("Key '%s' of pattern had no '*'", key);
+ die(_("key '%s' of pattern had no '*'"), key);
klen = kstar - key;
ksuffixlen = strlen(kstar + 1);
namelen = strlen(name);
@@ -690,7 +690,7 @@ static int match_name_with_pattern(const char *key, const
char *name,
struct strbuf sb = STRBUF_INIT;
const char *vstar = strchr(value, '*');
if (!vstar)
- die("Value '%s' of pattern has no '*'", value);
+ die(_("value '%s' of pattern has no '*'"), value);
strbuf_add(, value, vstar - value);
strbuf_add(, name + klen, namelen - klen - ksuffixlen);
strbuf_addstr(, vstar + 1);
@@ -995,12 +995,12 @@ static int match_explicit_lhs(struct ref *src,
* way to delete 'other' ref at the remote end.
*/
if (try_explicit_object_name(rs->src, match) < 0)
- return error("src refspec %s does not match any.",
rs->src);
+ return error(_("src refspec %s does not match any"),
rs->src);
if (allocated_match)
*allocated_match = 1;
return 0;
default:
- return error("src refspec %s matches more than one.", rs->src);
+ return error(_("src refspec %s matches more than one"),
rs->src);
}
}
@@ -1030,7 +1030,7 @@ static int match_explicit(struct ref *src, struct ref
*dst,
if (!dst_value ||
((flag & REF_ISSYMREF) &&
!starts_with(dst_value, "refs/heads/")))
- die("%s cannot be resolved to branch.",
+ die(_("%s cannot be resolved to branch"),
matched_src->name);
}
@@ -1041,30 +1041,30 @@ static int match_explicit(struct ref *src, struct ref
*dst,
if (starts_with(dst_value, "refs/"))
matched_dst = make_linked_ref(dst_value, dst_tail);
else if (is_null_oid(_src->new_oid))
- error("unable to delete '%s': remote ref does not
exist",
+ error(_("unable to delete '%s': remote ref does not
exist"),
dst_value);
else if ((dst_guess = guess_ref(dst_value, matched_src))) {
matched_dst = make_linked_ref(dst_guess, dst_tail);
free(dst_guess);
} else
- error("unable to push to unqualified destination: %s\n"
- "The destination refspec neither matches an "
- "existing ref on the remote nor\n"
- "begins with refs/, and we are unable to "
- "guess a prefix based on the source ref.",
+ error(_("unable to push to unqualified destination:
%s\n"
+
[PATCH v2 06/16] read-cache.c: mark more strings for translation
There are a couple other improvements on these strings as well:
- add missing colon (as separator)
- quote paths
- provide more information on error messages
- keep first word in lowercase
Signed-off-by: Nguyễn Thái Ngọc Duy
---
read-cache.c| 57 +
t/t1450-fsck.sh | 2 +-
2 files changed, 30 insertions(+), 29 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 0c37f4885e..858befe738 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -672,7 +672,8 @@ static struct cache_entry *create_alias_ce(struct
index_state *istate,
struct cache_entry *new_entry;
if (alias->ce_flags & CE_ADDED)
- die("Will not add file alias '%s' ('%s' already exists in
index)", ce->name, alias->name);
+ die(_("will not add file alias '%s' ('%s' already exists in
index)"),
+ ce->name, alias->name);
/* Ok, create the new entry using the name of the existing alias */
len = ce_namelen(alias);
@@ -687,7 +688,7 @@ void set_object_name_for_intent_to_add_entry(struct
cache_entry *ce)
{
struct object_id oid;
if (write_object_file("", 0, blob_type, ))
- die("cannot create an empty blob in the object database");
+ die(_("cannot create an empty blob in the object database"));
oidcpy(>oid, );
}
@@ -708,7 +709,7 @@ int add_to_index(struct index_state *istate, const char
*path, struct stat *st,
newflags |= HASH_RENORMALIZE;
if (!S_ISREG(st_mode) && !S_ISLNK(st_mode) && !S_ISDIR(st_mode))
- return error("%s: can only add regular files, symbolic links or
git-directories", path);
+ return error(_("%s: can only add regular files, symbolic links
or git-directories"), path);
namelen = strlen(path);
if (S_ISDIR(st_mode)) {
@@ -763,7 +764,7 @@ int add_to_index(struct index_state *istate, const char
*path, struct stat *st,
if (!intent_only) {
if (index_path(istate, >oid, path, st, newflags)) {
discard_cache_entry(ce);
- return error("unable to index file %s", path);
+ return error(_("unable to index file '%s'"), path);
}
} else
set_object_name_for_intent_to_add_entry(ce);
@@ -782,7 +783,7 @@ int add_to_index(struct index_state *istate, const char
*path, struct stat *st,
discard_cache_entry(ce);
else if (add_index_entry(istate, ce, add_option)) {
discard_cache_entry(ce);
- return error("unable to add %s to index", path);
+ return error(_("unable to add '%s' to index"), path);
}
if (verbose && !was_same)
printf("add '%s'\n", path);
@@ -793,7 +794,7 @@ int add_file_to_index(struct index_state *istate, const
char *path, int flags)
{
struct stat st;
if (lstat(path, ))
- die_errno("unable to stat '%s'", path);
+ die_errno(_("unable to stat '%s'"), path);
return add_to_index(istate, path, , flags);
}
@@ -818,7 +819,7 @@ struct cache_entry *make_cache_entry(struct index_state
*istate,
int len;
if (!verify_path(path, mode)) {
- error("Invalid path '%s'", path);
+ error(_("invalid path '%s'"), path);
return NULL;
}
@@ -844,7 +845,7 @@ struct cache_entry *make_transient_cache_entry(unsigned int
mode, const struct o
int len;
if (!verify_path(path, mode)) {
- error("Invalid path '%s'", path);
+ error(_("invalid path '%s'"), path);
return NULL;
}
@@ -1297,12 +1298,12 @@ static int add_index_entry_with_check(struct
index_state *istate, struct cache_e
if (!ok_to_add)
return -1;
if (!verify_path(ce->name, ce->ce_mode))
- return error("Invalid path '%s'", ce->name);
+ return error(_("invalid path '%s'"), ce->name);
if (!skip_df_check &&
check_file_directory_conflict(istate, ce, pos, ok_to_replace)) {
if (!ok_to_replace)
- return error("'%s' appears as both a file and as a
directory",
+ return error(_("'%s' appears as both a file and as a
directory"),
ce->name);
pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce),
ce_stage(ce));
pos = -pos-1;
@@ -1676,10 +1677,10 @@ static int verify_hdr(const struct cache_header *hdr,
unsigned long size)
int hdr_version;
if (hdr->hdr_signature != htonl(CACHE_SIGNATURE))
- return error("bad signature");
+ return error(_("bad signature 0x%08x"), hdr->hdr_signature);
hdr_version = ntohl(hdr->hdr_version);
if (hdr_version < INDEX_FORMAT_LB || INDEX_FORMAT_UB
[PATCH v2 11/16] repack: mark more strings for translation
Two strings are slightly updated to be consistent with the rest: die()
starts with lowercase.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/repack.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index c6a7943d5c..0af20fa0fc 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -197,7 +197,7 @@ static int write_oid(const struct object_id *oid, struct
packed_git *pack,
if (cmd->in == -1) {
if (start_command(cmd))
- die("Could not start pack-objects to repack promisor
objects");
+ die(_("could not start pack-objects to repack promisor
objects"));
}
xwrite(cmd->in, oid_to_hex(oid), GIT_SHA1_HEXSZ);
@@ -236,7 +236,7 @@ static void repack_promisor_objects(const struct
pack_objects_args *args,
char *promisor_name;
int fd;
if (line.len != 40)
- die("repack: Expecting 40 character sha1 lines only
from pack-objects.");
+ die(_("repack: Expecting 40 character sha1 lines only
from pack-objects."));
string_list_append(names, line.buf);
/*
@@ -247,13 +247,13 @@ static void repack_promisor_objects(const struct
pack_objects_args *args,
line.buf);
fd = open(promisor_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
if (fd < 0)
- die_errno("unable to create '%s'", promisor_name);
+ die_errno(_("unable to create '%s'"), promisor_name);
close(fd);
free(promisor_name);
}
fclose(out);
if (finish_command())
- die("Could not finish pack-objects to repack promisor objects");
+ die(_("could not finish pack-objects to repack promisor
objects"));
}
#define ALL_INTO_ONE 1
@@ -408,7 +408,7 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
out = xfdopen(cmd.out, "r");
while (strbuf_getline_lf(, out) != EOF) {
if (line.len != 40)
- die("repack: Expecting 40 character sha1 lines only
from pack-objects.");
+ die(_("repack: Expecting 40 character sha1 lines only
from pack-objects"));
string_list_append(, line.buf);
}
fclose(out);
@@ -417,7 +417,7 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
return ret;
if (!names.nr && !po_args.quiet)
- printf("Nothing new to pack.\n");
+ printf_ln(_("Nothing new to pack."));
/*
* Ok we have prepared all new packfiles.
@@ -477,13 +477,13 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
if (rollback_failure.nr) {
int i;
fprintf(stderr,
- "WARNING: Some packs in use have been renamed
by\n"
- "WARNING: prefixing old- to their name, in
order to\n"
- "WARNING: replace them with the new version of
the\n"
- "WARNING: file. But the operation failed, and
the\n"
- "WARNING: attempt to rename them back to
their\n"
- "WARNING: original names also failed.\n"
- "WARNING: Please rename them in %s
manually:\n", packdir);
+ _("WARNING: Some packs in use have been renamed
by\n"
+ "WARNING: prefixing old- to their name, in
order to\n"
+ "WARNING: replace them with the new version
of the\n"
+ "WARNING: file. But the operation failed,
and the\n"
+ "WARNING: attempt to rename them back to
their\n"
+ "WARNING: original names also failed.\n"
+ "WARNING: Please rename them in %s
manually:\n"), packdir);
for (i = 0; i < rollback_failure.nr; i++)
fprintf(stderr, "WARNING: old-%s -> %s\n",
rollback_failure.items[i].string,
--
2.19.1.1005.gac84295441
[PATCH v2 00/16] Mark more strings for translation
v2 splits non-trivial changes out to keep i18n patches simpler. A few
more word legos in fsck are removed. v2 also fixes a bug in fsck that
makes it print object id incorrectly.
Nguyễn Thái Ngọc Duy (16):
git.c: mark more strings for translation
alias.c: mark split_cmdline_strerror() strings for translation
archive.c: mark more strings for translation
attr.c: mark more string for translation
read-cache.c: turn die("internal error") to BUG()
read-cache.c: mark more strings for translation
read-cache.c: add missing colon separators
reflog: mark strings for translation
remote.c: turn some error() or die() to BUG()
remote.c: mark messages for translation
repack: mark more strings for translation
parse-options: replace opterror() with optname()
parse-options.c: turn some die() to BUG()
parse-options.c: mark more strings for translation
fsck: reduce word legos to help i18n
fsck: mark strings for translation
alias.c| 4 +-
archive.c | 8 +-
attr.c | 4 +-
builtin/fsck.c | 156 +
builtin/merge.c| 4 +-
builtin/reflog.c | 34
builtin/repack.c | 26 +++
builtin/revert.c | 3 +-
git.c | 32
parse-options-cb.c | 7 +-
parse-options.c| 64 ---
parse-options.h| 5 +-
read-cache.c | 73 -
ref-filter.c | 8 +-
remote.c | 49 ++--
t/t0040-parse-options.sh | 4 +-
t/t1410-reflog.sh | 6 +-
t/t1450-fsck.sh| 52 ++---
t/t4211-line-log.sh| 2 +-
t/t6050-replace.sh | 4 +-
t/t7415-submodule-names.sh | 6 +-
21 files changed, 292 insertions(+), 259 deletions(-)
--
2.19.1.1005.gac84295441
[PATCH v2 05/16] read-cache.c: turn die("internal error") to BUG()
Signed-off-by: Nguyễn Thái Ngọc Duy
---
read-cache.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index d57958233e..0c37f4885e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -316,7 +316,7 @@ static int ce_match_stat_basic(const struct cache_entry
*ce, struct stat *st)
changed |= DATA_CHANGED;
return changed;
default:
- die("internal error: ce_mode is %o", ce->ce_mode);
+ BUG("unsupported ce_mode: %o", ce->ce_mode);
}
changed |= match_stat_data(>ce_stat_data, st);
@@ -2356,14 +2356,14 @@ void validate_cache_entries(const struct index_state
*istate)
for (i = 0; i < istate->cache_nr; i++) {
if (!istate) {
- die("internal error: cache entry is not allocated from
expected memory pool");
+ BUG("cache entry is not allocated from expected memory
pool");
} else if (!istate->ce_mem_pool ||
!mem_pool_contains(istate->ce_mem_pool,
istate->cache[i])) {
if (!istate->split_index ||
!istate->split_index->base ||
!istate->split_index->base->ce_mem_pool ||
!mem_pool_contains(istate->split_index->base->ce_mem_pool, istate->cache[i])) {
- die("internal error: cache entry is not
allocated from expected memory pool");
+ BUG("cache entry is not allocated from expected
memory pool");
}
}
}
--
2.19.1.1005.gac84295441
[PATCH v2 04/16] attr.c: mark more string for translation
Signed-off-by: Nguyễn Thái Ngọc Duy
---
attr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/attr.c b/attr.c
index 60d284796d..3770bc1a11 100644
--- a/attr.c
+++ b/attr.c
@@ -372,8 +372,8 @@ static struct match_attr *parse_attr_line(const char *line,
const char *src,
if (strlen(ATTRIBUTE_MACRO_PREFIX) < namelen &&
starts_with(name, ATTRIBUTE_MACRO_PREFIX)) {
if (!macro_ok) {
- fprintf(stderr, "%s not allowed: %s:%d\n",
- name, src, lineno);
+ fprintf_ln(stderr, _("%s not allowed: %s:%d"),
+ name, src, lineno);
goto fail_return;
}
is_macro = 1;
--
2.19.1.1005.gac84295441
[PATCH v2 02/16] alias.c: mark split_cmdline_strerror() strings for translation
This function can be part of translated messages. To make sure we
don't have a sentence with mixed languages, mark the strings for
translation, but only use translated strings in places we know we will
output translated strings.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
alias.c | 4 ++--
builtin/merge.c | 2 +-
git.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/alias.c b/alias.c
index a7e4e57130..c471538020 100644
--- a/alias.c
+++ b/alias.c
@@ -47,8 +47,8 @@ void list_aliases(struct string_list *list)
#define SPLIT_CMDLINE_BAD_ENDING 1
#define SPLIT_CMDLINE_UNCLOSED_QUOTE 2
static const char *split_cmdline_errors[] = {
- "cmdline ends with \\",
- "unclosed quote"
+ N_("cmdline ends with \\"),
+ N_("unclosed quote")
};
int split_cmdline(char *cmdline, const char ***argv)
diff --git a/builtin/merge.c b/builtin/merge.c
index 4aa6071598..92ba7e1c6d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -577,7 +577,7 @@ static void parse_branch_merge_options(char *bmo)
argc = split_cmdline(bmo, );
if (argc < 0)
die(_("Bad branch.%s.mergeoptions string: %s"), branch,
- split_cmdline_strerror(argc));
+ _(split_cmdline_strerror(argc)));
REALLOC_ARRAY(argv, argc + 2);
MOVE_ARRAY(argv + 1, argv, argc + 1);
argc++;
diff --git a/git.c b/git.c
index 5fd30da093..c7e122cfc1 100644
--- a/git.c
+++ b/git.c
@@ -344,7 +344,7 @@ static int handle_alias(int *argcp, const char ***argv)
count = split_cmdline(alias_string, _argv);
if (count < 0)
die(_("bad alias.%s string: %s"), alias_command,
- split_cmdline_strerror(count));
+ _(split_cmdline_strerror(count)));
option_count = handle_options(_argv, , );
if (envchanged)
die(_("alias '%s' changes environment variables.\n"
--
2.19.1.1005.gac84295441
[PATCH v2 03/16] archive.c: mark more strings for translation
Two messages also print extra information to be more useful
Signed-off-by: Nguyễn Thái Ngọc Duy
---
archive.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/archive.c b/archive.c
index 9d16b7fadf..d8f6e1ce30 100644
--- a/archive.c
+++ b/archive.c
@@ -385,12 +385,12 @@ static void parse_treeish_arg(const char **argv,
int refnamelen = colon - name;
if (!dwim_ref(name, refnamelen, , ))
- die("no such ref: %.*s", refnamelen, name);
+ die(_("no such ref: %.*s"), refnamelen, name);
free(ref);
}
if (get_oid(name, ))
- die("Not a valid object name");
+ die(_("not a valid object name: %s"), name);
commit = lookup_commit_reference_gently(ar_args->repo, , 1);
if (commit) {
@@ -403,7 +403,7 @@ static void parse_treeish_arg(const char **argv,
tree = parse_tree_indirect();
if (tree == NULL)
- die("not a tree object");
+ die(_("not a tree object: %s"), oid_to_hex());
if (prefix) {
struct object_id tree_oid;
@@ -413,7 +413,7 @@ static void parse_treeish_arg(const char **argv,
err = get_tree_entry(>object.oid, prefix, _oid,
);
if (err || !S_ISDIR(mode))
- die("current working directory is untracked");
+ die(_("current working directory is untracked"));
tree = parse_tree_indirect(_oid);
}
--
2.19.1.1005.gac84295441
Re: Design of multiple hash support
On Mon, Nov 5, 2018 at 2:02 AM brian m. carlson wrote: > > I'm currently working on getting Git to support multiple hash algorithms > in the same binary (SHA-1 and SHA-256). In order to have a fully > functional binary, we'll need to have some way of indicating to certain > commands (such as init and show-index) that they should assume a certain > hash algorithm. > > There are basically two approaches I can take. The first is to provide > each command that needs to learn about this with its own --hash > argument. So we'd have: > > git init --hash=sha256 > git show-index --hash=sha256 > The other alternative is that we provide a global option to git, which > is parsed by all programs, like so: > > git --hash=sha256 init > git --hash=sha256 show-index I suppose this is about the "no repository/standalone" mode, because - it's hard to pass global arguments down to builtin commands (we often have to rely on global variables which are on the way out) - global options confuse new people and also harder to reorder (if you forget it, you have to alt-b all the way back to near the beginning of the command line and add it there, instead of near the end) - there aren't that many standalone commands I'm leaning towards "git foo --hash=". > There's also the question of what we want to call the option. The > obvious name is --hash, which is intuitive and straightforward. > However, the transition plan names the config option > extensions.objectFormat, so --object-format is also a possibility. If > we ever decide to support, say, zstd compression instead of zlib, we > could leverage the same option (say, --object-format=sha256:zstd) and > avoid the need for an additional option. This might be planning for a > future that never occurs, though. --object-format is less vague than --hash. The downside is it's longer (more to type) but I'm counting on git-completion.bash and the guess that people rarely need to use this option. -- Duy
Re: [PATCH 0/13] parseopt fixes from -Wunused-parameters
On Mon, Nov 5, 2018 at 7:49 PM Jeff King wrote:
>
> On Mon, Nov 05, 2018 at 05:51:07PM +0100, Duy Nguyen wrote:
>
> > On Mon, Nov 5, 2018 at 7:39 AM Jeff King wrote:
> > >
> > > Continuing my exploration of what -Wunused-parameters can show us, here
> > > are some bug-fixes related to parse-options callbacks.
> > >
> > > This is the last of the actual bug-fixes I've found. After this, I have
> > > about 60 patches worth of cleanups (i.e., dropping the unused
> > > parameters), and then I have a series to annotate parameters that must
> > > be unused (e.g., for functions that must conform to callback
> > > interfaces). After we can start compiling with -Wunused-parameters,
> > > assuming we don't find the annotations too cumbersome.
> >
> > Another good thing from this series is there are fewer --no-options to
> > complete.
> >
> > About the annotating unused parameters related to segfault bug fixes
> > in this series. Should we just add something like
> >
> > if (unset)
> > BUG("This code does not support --no-stuff");
> >
> > which could be done in the same patches that fix the segfault, and it
> > extends the diff context a bit to see what these callbacks do without
> > opening up the editor, and also serves as a kind of annotation?
>
> That's done in the final patch. I did originally do it alongside the
> actual segfault fixes, but since it needs doing in so many other
> callbacks, too, it made sense to me to do it all together.
Oops. I guess I stopped reading the series too early. Sorry for the noise.
--
Duy
Re: [PATCH] parse-options: deprecate OPT_DATE
On Mon, Nov 05, 2018 at 10:34:02AM -0800, Carlo Marcelo Arenas Belón wrote: > Signed-off-by: Carlo Marcelo Arenas Belón > --- > Documentation/technical/api-parse-options.txt | 4 > 1 file changed, 4 deletions(-) > > diff --git a/Documentation/technical/api-parse-options.txt > b/Documentation/technical/api-parse-options.txt > index 829b558110..2b036d7838 100644 > --- a/Documentation/technical/api-parse-options.txt > +++ b/Documentation/technical/api-parse-options.txt > @@ -183,10 +183,6 @@ There are some macros to easily define options: > scale the provided value by 1024, 1024^2 or 1024^3 respectively. > The scaled value is put into `unsigned_long_var`. > > -`OPT_DATE(short, long, _t_var, description)`:: > - Introduce an option with date argument, see `approxidate()`. > - The timestamp is put into `timestamp_t_var`. > - > `OPT_EXPIRY_DATE(short, long, _t_var, description)`:: > Introduce an option with expiry date argument, see > `parse_expiry_date()`. > The timestamp is put into `timestamp_t_var`. Thank you! I forgot to check the documentation. -Peff
Re: [PATCH 0/13] parseopt fixes from -Wunused-parameters
On Mon, Nov 05, 2018 at 05:51:07PM +0100, Duy Nguyen wrote:
> On Mon, Nov 5, 2018 at 7:39 AM Jeff King wrote:
> >
> > Continuing my exploration of what -Wunused-parameters can show us, here
> > are some bug-fixes related to parse-options callbacks.
> >
> > This is the last of the actual bug-fixes I've found. After this, I have
> > about 60 patches worth of cleanups (i.e., dropping the unused
> > parameters), and then I have a series to annotate parameters that must
> > be unused (e.g., for functions that must conform to callback
> > interfaces). After we can start compiling with -Wunused-parameters,
> > assuming we don't find the annotations too cumbersome.
>
> Another good thing from this series is there are fewer --no-options to
> complete.
>
> About the annotating unused parameters related to segfault bug fixes
> in this series. Should we just add something like
>
> if (unset)
> BUG("This code does not support --no-stuff");
>
> which could be done in the same patches that fix the segfault, and it
> extends the diff context a bit to see what these callbacks do without
> opening up the editor, and also serves as a kind of annotation?
That's done in the final patch. I did originally do it alongside the
actual segfault fixes, but since it needs doing in so many other
callbacks, too, it made sense to me to do it all together.
-Peff
[PATCH] parse-options: deprecate OPT_DATE
Signed-off-by: Carlo Marcelo Arenas Belón --- Documentation/technical/api-parse-options.txt | 4 1 file changed, 4 deletions(-) diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt index 829b558110..2b036d7838 100644 --- a/Documentation/technical/api-parse-options.txt +++ b/Documentation/technical/api-parse-options.txt @@ -183,10 +183,6 @@ There are some macros to easily define options: scale the provided value by 1024, 1024^2 or 1024^3 respectively. The scaled value is put into `unsigned_long_var`. -`OPT_DATE(short, long, _t_var, description)`:: - Introduce an option with date argument, see `approxidate()`. - The timestamp is put into `timestamp_t_var`. - `OPT_EXPIRY_DATE(short, long, _t_var, description)`:: Introduce an option with expiry date argument, see `parse_expiry_date()`. The timestamp is put into `timestamp_t_var`. -- 2.19.1.816.gcd69ec8cd
Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers)
Junio C Hamano writes:
> Anders Waldenborg writes:
>
>> @@ -1352,6 +1353,17 @@ static size_t format_commit_one(struct strbuf *sb, /*
>> in UTF-8 */
>> arg++;
>>
>> opts.only_trailers = 1;
>> +} else if (skip_prefix(arg, "separator=",
>> )) {
>> +size_t seplen = strcspn(arg, ",)");
>> +strbuf_reset();
>> +char *fmt = xstrndup(arg, seplen);
>> +strbuf_expand(, fmt,
>> format_fundamental, NULL);
>
> This somehow feels akin to using end-user supplied param to printf(3)
> as its format argument e.g.
>
> int main(int ac, char *av) {
> printf(av[1]);
> return 0;
> }
>
> which is not a good idea. Is there a mechanism with which we can
> ensure that the separator= specification will never come from
> potentially malicious sources (e.g. not used to show things on webpage
> allowing random folks who access he site to supply custom format)?
I can't see a case where this could add anything that isn't already
possible.
AFAICU strbuf_expand doesn't suffer from the worst things that printf(3)
suffers from wrt untrusted format string (i.e no printf style %n which
can write to memory, and no vaargs on stack which allows leaking random
stuff).
The separator option is part of the full format string. If a malicious
user can specify that, they can't really do anything new, as the
separator only can expand %n and %xNN, which they already can do in the
full string.
But maybe I'm missing something?
Re: [PATCH] diff: differentiate error handling in parse_color_moved_ws
On Sun, Nov 4, 2018 at 10:12 PM Junio C Hamano wrote:
>
> Junio C Hamano writes:
>
> > Stefan Beller writes:
> >
> >>
> >> -static int parse_color_moved_ws(const char *arg)
> >> +static unsigned parse_color_moved_ws(const char *arg)
> >> {
> >> int ret = 0;
> >> struct string_list l = STRING_LIST_INIT_DUP;
> >> @@ -312,15 +312,19 @@ static int parse_color_moved_ws(const char *arg)
> >> ret |= XDF_IGNORE_WHITESPACE;
> >> else if (!strcmp(sb.buf, "allow-indentation-change"))
> >> ret |= COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE;
> >> -else
> >> +else {
> >> +ret |= COLOR_MOVED_WS_ERROR;
> >> error(_("ignoring unknown color-moved-ws mode '%s'"),
> >> sb.buf);
> >> +}
> >> ...
> >> } else if (skip_prefix(arg, "--color-moved-ws=", )) {
> >> -options->color_moved_ws_handling = parse_color_moved_ws(arg);
> >> +unsigned cm = parse_color_moved_ws(arg);
> >> +if (cm & COLOR_MOVED_WS_ERROR)
> >> +die("bad --color-moved-ws argument: %s", arg);
> >> +options->color_moved_ws_handling = cm;
> >
> > Excellent.
> >
> > Will queue. Perhaps a test or two can follow to ensure a bad value
> > from config does not kill while a command line does?
>
> Wait. This does not fix
>
> git -c diff.colormovedws=nonsense diff
>
> that dies with an error message---it should ignore the config and at
> moat issue a warning.
$ git -c core.abbrev=41 diff
error: abbrev length out of range: 41
fatal: unable to parse 'core.abbrev' from command-line config
$ ./git -c diff.colormovedws=nonsense diff HEAD
error: ignoring unknown color-moved-ws mode 'nonsense'
fatal: unable to parse 'diff.colormovedws' from command-line config
Ah, I see the issue there. We actually have to return 'success' to the
config machinery after the warning claiming ignoring the setting or
we'd have to reword the warning to state we're not ignoring the bogus
setting.
> The command line handling of
>
> git diff --color-moved-ws=nonsense
>
> does correctly die, but it first says "error: ignoring" before
> saying "fatal: bad argument", which is suboptimal.
So to find the analogous here, maybe:
$ git diff --color=bogus
error: option `color' expects "always", "auto", or "never"
> So, not so excellent (yet) X-<.
So to reach excellence, we'd want to reword the warning
message and a test.
Thanks,
Stefan
Re: git-rebase is ignoring working-tree-encoding
On Mon, Nov 05, 2018 at 05:24:39AM +0100, Adrián Gimeno Balaguer wrote: [] > https://github.com/git/git/pull/550 [] > This is covered in the mentioned PR above. Thanks for feedback. Thanks for the code, I will have a look (the next days) > > -- > Adrián
Re: Design of multiple hash support
On Sun, Nov 4, 2018 at 6:36 PM Junio C Hamano wrote: > > "brian m. carlson" writes: > > > I'm currently working on getting Git to support multiple hash algorithms > > in the same binary (SHA-1 and SHA-256). In order to have a fully > > functional binary, we'll need to have some way of indicating to certain > > commands (such as init and show-index) that they should assume a certain > > hash algorithm. > > > > There are basically two approaches I can take. The first is to provide > > each command that needs to learn about this with its own --hash > > argument. So we'd have: > > > > git init --hash=sha256 > > git show-index --hash=sha256 > > > The other alternative is that we provide a global option to git, which > > is parsed by all programs, like so: > > > > git --hash=sha256 init > > git --hash=sha256 show-index > I am assuming that "show-index" above is a typo for something like > "hash-object"? Actually both seem plausible, as both do not require RUN_SETUP, which means they cannot rely on the extensions.objectFormat setting. When having a global setting, would that override the configured object format extension in a repository, or do we error out? So maybe git -c extensions.objectFormat=sha256 init is the way to go, for now? (Are repository format extensions parsed just like normal config, such that non-RUN_SETUP commands can rely on the (non-)existence to determine whether to use the default or the given hash function?) > It is hard to answer the question without knowing what exactly does > "(to) support multiple hash algorithms" mean. For example, inside > today's repository, what should this command do? > > git --hash=sha256 cat-file commit HEAD There is a section "Object names on the command line" in Documentation/technical/hash-function-transition.txt and I assume that this before the "dark launch" phase, so I would expect the latter to work (no error but conversion/translation on the fly) eventually as a goal. But the former might be in scope of one series. > It can work this way: > > - read HEAD, discover that I am on 'master' branch, read refs/heads/master >to learn the object name in 40-hex, realize that it cannot be >sha256 and report "corrupt ref". > > Or it can work this way: > > - read repository format, realize it is a good old sha1 repository. > > - do the usual thing to get to read_object() to read the commit >object data for the commit at HEAD, doing all of it in sha1. > > - in the commit object data, locate references to other objects >that use sha1 name. > > - replace these sha1 references with their sha256 counterparts and >show the result. > > I am guessing that you are doing the former as a good first step, in > which case, as an option that changes/affects the behaviour of git > globally, I think "git --hash=sha256" would make sense, like other > global options like --literal-pathspecs and --no-replace-objects. > > Thanks.
Re: [PATCH 12/12] fsck: mark strings for translation
On Mon, Oct 29, 2018 at 12:53 PM SZEDER Gábor wrote:
> The contents of 'out':
>
> broken link fromtree be45bbd3809e0829297cefa576e699c134abacfd
> (refs/heads/master@{1112912113}:caesar.t)
> toblob be45bbd3809e0829297cefa576e699c134abacfd
> (refs/heads/master@{1112912113}:caesar.t)
> missing blob be45bbd3809e0829297cefa576e699c134abacfd
> (refs/heads/master@{1112912113}:caesar.t)
>
> On a related (side)note, I think it would be better if this 'grep'
I found the problem. Some function returns static string which works
fine when we do two printf()'s, one call of that function per
printf(). But when combining two printf() in one, we have a problem.
Thanks for catching.
--
Duy
Re: [PATCH 0/13] parseopt fixes from -Wunused-parameters
On Mon, Nov 5, 2018 at 7:39 AM Jeff King wrote:
>
> Continuing my exploration of what -Wunused-parameters can show us, here
> are some bug-fixes related to parse-options callbacks.
>
> This is the last of the actual bug-fixes I've found. After this, I have
> about 60 patches worth of cleanups (i.e., dropping the unused
> parameters), and then I have a series to annotate parameters that must
> be unused (e.g., for functions that must conform to callback
> interfaces). After we can start compiling with -Wunused-parameters,
> assuming we don't find the annotations too cumbersome.
Another good thing from this series is there are fewer --no-options to complete.
About the annotating unused parameters related to segfault bug fixes
in this series. Should we just add something like
if (unset)
BUG("This code does not support --no-stuff");
which could be done in the same patches that fix the segfault, and it
extends the diff context a bit to see what these callbacks do without
opening up the editor, and also serves as a kind of annotation?
--
Duy
Re: [PATCH] multi-pack-index: make code -Wunused-parameter clean
On 11/3/2018 10:27 PM, Jeff King wrote:
On Sat, Nov 03, 2018 at 05:49:57PM -0700, Carlo Marcelo Arenas Belón wrote:
introduced in 662148c435 ("midx: write object offsets", 2018-07-12)
but included on all previous versions as well.
midx.c:713:54: warning: unused parameter 'nr_objects' [-Wunused-parameter]
likely an oversight as the information needed to iterate over is
embedded in nr_large_offset
I've been preparing a series to make the whole code base compile with
-Wunused-parameter, and I handled this case a bit differently.
-- >8 --
Subject: [PATCH] midx: double-check large object write loop
The write_midx_large_offsets() function takes an array of object
entries, the number of entries in the array (nr_objects), and the number
of entries with large offsets (nr_large_offset). But we never actually
use nr_objects; instead we keep walking down the array and counting down
nr_large_offset until we've seen all of the large entries.
This is correct, but we can be a bit more defensive. If there were ever
a mismatch between nr_large_offset and the actual set of large-offset
objects, we'd walk off the end of the array.
Since we know the size of the array, we can use nr_objects to make sure
we don't walk too far.
Signed-off-by: Jeff King
Thanks, both, for catching this. I prefer the approach that adds defenses.
Reviewed-by: Derrick Stolee
---
midx.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/midx.c b/midx.c
index 4fac0cd08a..ecd583666a 100644
--- a/midx.c
+++ b/midx.c
@@ -712,12 +712,18 @@ static size_t write_midx_object_offsets(struct hashfile
*f, int large_offset_nee
static size_t write_midx_large_offsets(struct hashfile *f, uint32_t
nr_large_offset,
struct pack_midx_entry *objects,
uint32_t nr_objects)
{
- struct pack_midx_entry *list = objects;
+ struct pack_midx_entry *list = objects, *end = objects + nr_objects;
size_t written = 0;
while (nr_large_offset) {
- struct pack_midx_entry *obj = list++;
- uint64_t offset = obj->offset;
+ struct pack_midx_entry *obj;
+ uint64_t offset;
+
+ if (list >= end)
+ BUG("too many large-offset objects");
+
+ obj = list++;
+ offset = obj->offset;
if (!(offset >> 31))
continue;
Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines
On Mon, Nov 5, 2018 at 1:56 AM Junio C Hamano wrote: > > Nguyễn Thái Ngọc Duy writes: > > > A reverted commit will have a new trailer > > > > Revert: > > Please don't, unless you are keeping the current "the effect of > commit X relative to its parent Y was reverted" writtein in prose, > which is meant to be followed up with a manually written "because > ..." and adding this as an extra footer that is meant solely for > machine consumption. Of course reversion of a merge needs to say > relative to which parent of the merge it is undoing. I think the intent of writing "This reverts " to encourage writing "because ..." is good, but in practice many people just simply not do it. And by not describing anything at all (footers don't count) some commit hook can force people to actually write something. But for the transition period I think we need to keep both anyway, whether this "This reverts ..." should stay could be revisited another day (or not, even). > > Similarly a cherry-picked commit with -x will have > > > > Cherry-Pick: > > Unlike the "revert" change above, this probably is a good change, as > a"(cherry-pickt-from: X)" does not try to convey anything more or > anything less than such a standard looking trailer and it is in > different shape only by historical accident. But people's scripts > may need to have a long transition period for this change to happen. Yep. I'll code something up to print both by default with config knobs to disable either. Unless you have some better plan of course. -- Duy
Re: [PATCH/RFC v2] sequencer.c: record revert/cherry-pick commit with trailer lines
On Sun, Nov 4, 2018 at 10:30 PM brian m. carlson wrote: > However, I do have concerns about breaking compatibility with existing > scripts. I wonder if we could add a long alias for git cherry-pick -x, > say "--notate" and have "--notate=text" mean "-x" and "--notate=trailer" > mean this new format. Similarly, git revert could learn such an option > as well. I don't think it will help unless you are the only developer on some repo. If you have some scripts parsing the old format, people could choose to commit using the new format anyway and your scripts will have to adapt (it's too late to revert because it's already part of git history). The transition plan could be outputing both old and new formats at the same time (optionally allowing to disable the old one) and leave it like that for a couple releases. Then we could stop producing the old output and hope that all the scripts in the world have caught up. Not a great plan. > One final thought: since our trailers seem to act as if we wrote "this > commit" (has been), I wonder if we should say "Reverts" instead of > "Revert" for consistency. Yeah that or Reverting:, both are better than Revert:. I guess I'll just go with Reverts: if this patch moves forward. -- Duy
Re: "git checkout" safety feature
On Mon, Nov 5, 2018 at 7:53 AM Jeff King wrote: > > On Mon, Nov 05, 2018 at 07:24:42AM +0100, Matthias Urlichs wrote: > > > Hi, > > > "git checkout " is a feature to overwrite local > > > changes. It is what you use when you make a mess editing the files > > > and want to go back to a known state. Why should that feature be > > > destroyed? > > > > Not destroyed, but optionally made finger-fumble-save – like "alias rm > > rm -i". > > There are a couple of destructive commands left in Git (e.g., this one, > and "git reset --hard" is another). I didn't dig up archive references, > but the topic of safety valves has come up many times over the years. > The discussion usually ends with the notion that instead of warning > that the operation is destructive (because that gets annoying when its > purpose is to be destructive), we should make it possible to undo a > mistake. > > So in this case, that would mean saving the working tree file to a blob > before we obliterate it. > > See similar discussion in: > > > https://public-inbox.org/git/cacsjy8c5qolvg4pzy_pthqoygh9ohdevhxsuywqhqypn3ob...@mail.gmail.com/ > > for example. That work is still ongoing (slowly). I realized that reflog code was buried deep in files-backend.c and would not make sense to reuse in its current form. So I had to move the code to a common place, which adds more work. But it will be coming! Hopefully before 2020 at my usual development speed. While we're at it, I've been running something with that "index reflog" (no pruning) for a month with lots of "add -p" and the file size is just 163KB, so the reflog format seems promising for this purpose. -- Duy
Re: [RFC v1] Add virtual file system settings and hook proc
On Mon, Nov 5, 2018 at 12:40 PM Ævar Arnfjörð Bjarmason wrote: > > > On Sun, Nov 04 2018, Duy Nguyen wrote: > > > On Wed, Oct 31, 2018 at 9:53 PM Ben Peart wrote: > >> >> +core.virtualFilesystem:: > >> >> + If set, the value of this variable is used as a command which > >> >> + will identify all files and directories that are present in > >> >> + the working directory. Git will only track and update files > >> >> + listed in the virtual file system. Using the virtual file > >> >> system > >> >> + will supersede the sparse-checkout settings which will be > >> >> ignored. > >> >> + See the "virtual file system" section of linkgit:githooks[6]. > >> > > >> > It sounds like "virtual file system" is just one of the use cases for > >> > this feature, which is more about a dynamic source of sparse-checkout > >> > bits. Perhaps name the config key with something along sparse checkout > >> > instead of naming it after a use case. > >> > >> It's more than a dynamic sparse-checkout because the same list is also > >> used to exclude any file/folder not listed. That means any file not > >> listed won't ever be updated by git (like in 'checkout' for example) so > >> 'stale' files could be left in the working directory. It also means git > >> won't find new/untracked files unless they are specifically added to the > >> list. > > > > OK. I'm not at all interested in carrying maintenance burden for some > > software behind closed doors. I could see values in having a more > > flexible sparse checkout but this now seems like very tightly designed > > for GVFS. So unless there's another use case (preferably open source) > > for this, I don't think this should be added in git.git. > > I haven't looked at the patch in any detail beyond skimming it, and > you're more familiar with this area... > > But in principle I'm very interested in getting something like this in > git.git, even if we were to assume GVFS was a 100% proprietary > implementation. I have nothing against building a GVFS-like solution. If what's submitted can be the building blocks for that, great! But if it was just for GVFS (and it was not available to everybody) then no thank you. -- Duy
Re: [RFC v1] Add virtual file system settings and hook proc
On Sun, Nov 4, 2018 at 10:01 PM brian m. carlson wrote: > > On Sun, Nov 04, 2018 at 07:34:01AM +0100, Duy Nguyen wrote: > > On Wed, Oct 31, 2018 at 9:53 PM Ben Peart wrote: > > > It's more than a dynamic sparse-checkout because the same list is also > > > used to exclude any file/folder not listed. That means any file not > > > listed won't ever be updated by git (like in 'checkout' for example) so > > > 'stale' files could be left in the working directory. It also means git > > > won't find new/untracked files unless they are specifically added to the > > > list. > > > > OK. I'm not at all interested in carrying maintenance burden for some > > software behind closed doors. I could see values in having a more > > flexible sparse checkout but this now seems like very tightly designed > > for GVFS. So unless there's another use case (preferably open source) > > for this, I don't think this should be added in git.git. > > I should point out that VFS for Git is an open-source project and will > likely have larger use than just at Microsoft. There are both Windows > and Mac clients and there are plans for a Linux client as well. > Ideally, it would work with an unmodified upstream Git, which is (I > assume) why Ben is sending this series. Ah I didn't know that. Thank you. I'll have to look at this GVFS some time then. If we're going to support GVFS though, I think there should be a big (RFC perhaps) series that includes everything to at least give an overview what the end game looks like. Then it could be split up into smaller series. -- Duy
Re: [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:"
Ævar Arnfjörð Bjarmason writes:
> After all sometimes "other" is just the repo on my laptop or server. I
> shouldn't need to jump through hoops to re-push stuff from my "other"
> repo anymore than from the local repo.
>
> Yes refs/remotes/* isn't guaranteed to be "other repo's branches" in the
> same way our local refs/heads/* is, and this series bends over backwards
> to check if someone's (ab)used it for that, but I think for the purposes
> of the default UI we should treat it as "branches from other remotes" if
> we find commit objects there.
I do not think there is *any* disagreement between us that
refs/remotes/* is where somebody else's branches are stored.
The fundamental difference between us is what this "push" is doing.
You are limiting yourself to a single use case where you push to
publish to the remote repository, as if the work on somebody else's
commit were exactly like your own, hence you believe the push should
go to refs/heads/* of the receiving repository.
I am also allowing an additional use case I often have to use, where
I emulate a fetch done at the receiving repository by pushing into it.
I have refs/remotes/* by fetching somebody else's branches locally,
and instead of doing the same fetch over there by logging into it
and doing "git fetch" interactively, I push refs/remotes/* I happen
to have locally into refs/remotes/* there.
And I happen to think both are equally valid workflows, and there is
no strong reason to think everybody would intuitively expect a push
of my local refs/remotes/* will go to refs/heads/* of the receiving
repository like you do. I'd rather want to make sure we do not make
wrong guesses and force the user to "recover".
> So I think the *only* thing we should be checking for the purposes of
> this DWYM feature is what local type of object (branch or tag) we find
> on the LHS of the refspec. And if we're sure of its type push to
> refs/{heads,tags}/* as appropriate.
That is insufficient as long as you do not consider refs/remotes/*
as one of the equally valid possibilities that sits next to refs/heads
and res/tags/.
Re: if YOU use a Windows GUI for Git, i would appreciate knowing which one and why
Hi Gerry, I'll give my view, as someone approaching retirement, but who worked as an Engineer in a mainly Windows environment. On 04/11/2018 17:48, _g e r r y _ _l o w r y _ wrote: PREAMBLE [START] - please feel free to skip this first section Forgive me for asking this question on a mailing list. stackoverflow would probably kill such a question before the bits were fully saved to a server drive. Let me explain why i am asking and why i am not being a troll. [a] i'm "old school", i.e., > 50% on my way to being age 72 [born 1947] 8 years behind.. [b] when i started programming in 1967, most of my work input was via punched cards '69, at school, post/compile/run/wait for post; 1 week (Maths club) [c] punching my own cards was cool Pin punching individual chads ;-) [d] IBM System/360 mainframe assembler was cool and patching previously punched card encoded machine code output was a fun risky but at times necessary challenge. Eventually the 370 at university. [e] using command windows and coding batch files for Gary Kildall's CP/M and the evil empire's PC/MS-DOS was how i accomplished many tasks for early non-GUI environments (i still continue this practice even in Windows 10 (a.k.a. please don't update my PC O/S behind my back again versions of MS Windows)). Engineer in electronics; software was an interlinked part of electronics back then [f] my introduction to Git was via a command line based awesome video that has disappeared (i asked this community about that in a previous thread). Discovered in 2011 via 'Code News' article - Spotted immediately that it solved the engineers version control issue because it 'distributed' the control. I've tried a few of the Gui's. BOTTOM LINE: virtually 100% of my Git use has been via Git Bash command line [probably downloaded from https://git-scm.com/] For me, and i suspect even for most people who live with GUI platforms, [a well kept secret fact] using the keyboard is faster than using the mouse [especially when one's fingers are already over one's keyboard-example, closing one or more "windows" via Alt+F4. Also for me, i am happy to change some code and/or write some new code, Alt+Tab to Git Bash frequently, ADD/COMMIT, then Alt+Tab back to whatever IDE i'm using [mostly LINQPad and vs2017]; i know that's quite a bit schizophrenic of me-command line Git but GUI IDE. PREAMBLE [END] QUESTION: if YOU use a Windows GUI for Git, i would appreciate knowing which one and why i have been asked to look at GUI versions of Git for Windows. I presume that this is for a client who isn't sure what they want http://www.abilitybusinesscomputerservices.com/home.html https://git-scm.com/download/gui/windows currently lists 22 options. That's nearly as bad as choosing a Linux distro ;-) if i had more time left in my life and the option, because of my own nature, i'd likely download and evaluate all 22 - Mr.T would pity the fool that i often can be. CAUTION: i am not looking for anyone to disparage other Git Windows GUIs. Let me break down the question into 4 parts: [1a] Which do you prefer: Git GUI, Git command line? I use the three parts provided as part of regular Git and Git for Windows, that is git-gui, gitk and git cli in a terminal (mintty) [1b] What is your reason for your [1a] preference? I have been in a general Windows environment for decades. The Gui format with single buttons/drop downs that do one thing well, without finger trouble side effects, is good in such environments. One cannot be master of everything. The cli is good for specialists and special actions, especially precision surgery. The key is to avoid the "the surgery was a success but the patient died" results. [2a] if applicable, which Git GUI do you prefer? git-gui and gitk are now the only two I use. [2b] What is your reason for your [2a] preference? Many of the other Gui's hide the power of Git and its new abstraction of no longer actually being about "Control" (by 'management'). Now it is about veracity. If you have the right object ID (sha1/sha256) you have an identical original [there are no 'copies', all Mona Lisas with the hash are the same]. Management can choose which hash to accept upstream. Most other Gui's try to hide behind the old school Master-copy view point that was developed in the 19th century for drawing office control. If you damaged the master drawing the ability to make things and do business was lost. Protecting the master drawing was everything. They were traced before they went to the blue print machine. Changes were batched up before the master could be touched (that risk again). Too may Gui's (and their Managements!) still try to work the old way, loosing all the potential benefits. They are still hammer wielders looking for nails, and only finding screws to smash. I've heard reasonable things about SmartGit but that costs money so I
Re: [RFC v1] Add virtual file system settings and hook proc
On Sun, Nov 04 2018, Duy Nguyen wrote: > On Wed, Oct 31, 2018 at 9:53 PM Ben Peart wrote: >> >> +core.virtualFilesystem:: >> >> + If set, the value of this variable is used as a command which >> >> + will identify all files and directories that are present in >> >> + the working directory. Git will only track and update files >> >> + listed in the virtual file system. Using the virtual file system >> >> + will supersede the sparse-checkout settings which will be ignored. >> >> + See the "virtual file system" section of linkgit:githooks[6]. >> > >> > It sounds like "virtual file system" is just one of the use cases for >> > this feature, which is more about a dynamic source of sparse-checkout >> > bits. Perhaps name the config key with something along sparse checkout >> > instead of naming it after a use case. >> >> It's more than a dynamic sparse-checkout because the same list is also >> used to exclude any file/folder not listed. That means any file not >> listed won't ever be updated by git (like in 'checkout' for example) so >> 'stale' files could be left in the working directory. It also means git >> won't find new/untracked files unless they are specifically added to the >> list. > > OK. I'm not at all interested in carrying maintenance burden for some > software behind closed doors. I could see values in having a more > flexible sparse checkout but this now seems like very tightly designed > for GVFS. So unless there's another use case (preferably open source) > for this, I don't think this should be added in git.git. I haven't looked at the patch in any detail beyond skimming it, and you're more familiar with this area... But in principle I'm very interested in getting something like this in git.git, even if we were to assume GVFS was a 100% proprietary implementation (which it's not, it's open source[1] and there's work on a non-Windows port[2]). By some definition we already support at least one "virtual filesystem", or two. I.e. we check files into a *.git repository where everything's trees & blobs, and then we need to ferry it back & forth between that and a POSIX-like fs using readdir(), stat(), open() and the like. Of course this isn't "virtual" in the fs sense, but it's a foreign virtual FS as far as git is concerned. Our other "virtual filesystem" (although this is a stretch) is what we expose via the plumbing commands. Both of these, as covered at length in various GVFS design docs / background info, aren't a good fit for some implementation where you'd like a file-like view that works with the index/status etc. but doesn't involve checking out everything. There's a huge gap to bridge between our plumbing & the features we expose that require a POSIX-like fs. I can see such an ability being very useful for things that aren't the massive repo Microsoft is interested in supporting. E.g. even for a ~500MB repo it's a stretch to clone that, edit it, and submit a PR on something like an iOS/Android device. Once we have something in git to support the likes of GVFS supporting use-cases like that also becomes easier. Between shallow cloning, the various object filters & this it would be great to be able to get a 1MB clone of git.git on my phone and submit a patch to it. The only potential downside I see is that there's currently exactly one implementation of this sort of thing in the wild, so we risk any such API becoming too tied up with just what GVFS wants, and not what we'd like to support with such a thing in general. This is what e.g. the w3c tries to avoid with having multiple browser implementations before something is standardized. But I think that's easy to work around. E.g. we can document any such API saying that it's an experimental v1 and is on probation until we get more users of the feature and figure out the warts & limitations. If it does end up being too GVFS-specific we could make a breaking change at some point and eventually drop the v1 version. 1. https://gvfs.io/ 2. https://arstechnica.com/gadgets/2017/11/microsoft-and-github-team-up-to-take-git-virtual-file-system-to-macos-linux/
Re: [PATCH v5 10/12] Add a base implementation of SHA-256 support
On Sun, Nov 04 2018, brian m. carlson wrote:
> SHA-1 is weak and we need to transition to a new hash function. For
> some time, we have referred to this new function as NewHash. Recently,
> we decided to pick SHA-256 as NewHash. The reasons behind the choice of
> SHA-256 are outlined in the thread starting at [1] and in the commit
> history for the hash function transition document.
Nit: In some contradiction now to what's said in
hash-function-transition.txt, see 5988eb631a ("doc
hash-function-transition: clarify what SHAttered means", 2018-03-26).
> + {
> + "sha256",
> + /* "s256", big-endian */
The existing entry/comment for sha1 is:
"sha1",
/* "sha1", big-endian */
So why the sha256/s256 difference in the code/comment? Wondering if I'm
missing something and we're using "s256" for something.
> const char *empty_tree_oid_hex(void)
> diff --git a/sha256/block/sha256.c b/sha256/block/sha256.c
> [...]
I had a question before about whether we see ourselves perma-forking
this implementation based off libtomcrypt, as I recall you said yes.
Still, I think it would be better to introduce this in at least two-four
commits where the upstream code is added as-is, then trimmed down to
size, then adapted to our coding style, and finally we add our own
utility functions.
It'll make it easier to forward-port any future upstream changes.
> + perl -E "for (1..10) { print q{aa}; }" | \
> + test-tool sha256 >actual &&
> + grep cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0
> actual &&
> + perl -E "for (1..10) { print q{abcdefghijklmnopqrstuvwxyz}; }" | \
> + test-tool sha256 >actual &&
I've been wanting to make use depend on perl >= 5.10 (previous noises
about that on-list), but for now we claim to support >=5.8, which
doesn't have the -E switch.
But most importantly you aren't even using -E features here, and this
isn't very idoimatic Perl. Instead do, respectively:
perl -e 'print q{aa} x 10'
perl -e "print q{abcdefghijklmnopqrstuvwxyz} x 10"
Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
On Sat, Nov 03 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > >> Although I'm on the fence with the approach in 1/5. Should this be a >> giant getopt switch statement like that in a helper script? >> >> An alternative would be to write out a shell file similar to >> GIT-BUILD-OPTIONS and source that from this thing. I don't know, >> what do you all think? > > Not really. Why do we iterate over these in a shell loop, rather > than having make to figure them out, just like we do when we "loop > over the source files and turn them into object files" without using > a shell loop? What's so special about enumerating the installation > targets and iterating over the enumeration to perform an action on > each of them? I think that is the first question we should be > asking before patch 1/5, which already assumes that it has been > decided that it must be done with a shell loop. > > I think "first install 'git' itself, and then make these > other things derived from it" should let $(MAKE) install > things in parallel just like it can naturally do many things > in parallel, and the dependency rule to do so should not be > so bad, I suspect. > > This is a tangent, but I have long been wishing that somebody would > notice that output during install and (dist)clean without V=1 is so > different from the normal targets and do something about it, and > hoped that that somebody finally turned out to be you doing so in > this series X-<. I'm all for this, but don't have enough Make skills to make it happen. Can you or someone else post a WIP patch showing how to do this? What would the targets look like? Something that's a build target for the target file in the installation directory, so e.g. if you ran "all install" and had modified just one file (and no recursive rebuilds) you'd install just that one file? Early on in the "install" target we install many of these programs, and then some of these for-loops re-install on top of them. Actually now that I think of this this is one of the reasons for the 2>/dev/null probably, i.e. run "install" twice and you don't want to get errors. Anyway, regardless of how the for-loop looks like (shell or make-powered) I split this up because it was getting really hard to maintain the *inner* part of those loops. I.e. needing to specially quote everything, end lines with \ etc. But reading on... >> I'd like to say it's ready, but I've spotted some fallout: > > I still have not recovered from the trauma I suffered post 1.6.0 > era, so I would rather *not* engage in a long discussion like this > one (it is a long thread; reserve a solid hour to read it through if > you are interested), > > https://public-inbox.org/git/alpine.lfd.1.10.0808261435470.3...@nehalem.linux-foundation.org/ > > which would be needed to defend the choice, if we decide to omit > installing the git-foo on disk in a released version. Thanks. I'll read that later. > I personally have no objection to offer a knob that can e used to > force installation of symlinks without falling back to other > methods. I think it would be ideal to do so without special casing > symbolic links---rather, it would be ideal if it were a single knob > INSTALL_GIT_FOO_METHOD=(symlinks|hardlinks|copies) that says "I want > them to be installed as (symlinks|hardlinks|copies), no fallbacks". ... If you're happy to accept a patch that rips out this whole conditional fallback logic and just makes it an if/elsif/elsif for symlink/hardlink/copy this makes things a lot easier. >> Ævar Arnfjörð Bjarmason (5): >> Makefile: move long inline shell loops in "install" into helper >> Makefile: conform some of the code to our coding standards >> Makefile: stop hiding failures during "install" >> Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch >> Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag >> >> Makefile | 65 +++-- >> install_programs | 124 +++ >> 2 files changed, 151 insertions(+), 38 deletions(-) >> create mode 100755 install_programs
Re: [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:"
I'll re-roll this. Hopefully sooner than later. I'll leave out the later
part of this series as it's more controversial and we can discuss that
later on its own. Meanwhile just some replies to this (while I
remember):
> Ævar Arnfjörð Bjarmason writes:
>
>>> On the other hand, I do not think I mind all that much if a src that
>>> is a tag object to automatically go to refs/tags/ (having a tag
>>> object in refs/remotes/** is rare enough to matter in the first
>>> place).
>>
>> Yeah maybe this is going too far. I don't think so, but happy to me
>> challenged on that point :)
>>
>> I don't think so because the only reason I've ever needed this is
>> because I deleted some branch accidentally and am using a push from
>> "remotes/*" to bring it back. I.e. I'll always want branch-for-branch,
>> not to push that as a tag.
>
> Oh, I didn't consider pushing it out as a tag, but now you bring it
> up, I think that it also would make sense in a workflow to tell your
> colleages to look at (sort of like how people use pastebin---"look
> here, this commit has the kind of change I have in mind in this
> discussion") some random commit and the commit happens to be sitting
> at a tip of a remote-trackig branch. Instead of pushing it out as a
> branch or a remote-tracking branch, which has strong connotations of
> inviting others to build on top, pushing it out as a tag would make
> more sense in that context.
>
> And as I mentioned already, I think it would equally likely, if not
> more likely, for people like me to push remotes/** out as a
> remote-tracking branch (rather than a local branch) of the
> repository I'm pushing into.
>
> So I tend to agree that this is going too far. If the original
> motivating example was not an ingredient of everyday workflow, but
> was an one-off "recovery", I'd rather see people forced to be more
> careful by requiring "push origin/frotz:refs/heads/frotz" rather
> than incorrectly DWIDNM "push origin/frotz:frotz" and ending up with
> creating refs/tags/frotz or refs/remotes/origin/frotz, which also
> are plausible choices depending on what the user is trying to
> recover from, which the sending end would not know (the side on
> which the accidental loss of a ref happened earlier is on the remote
> repository that would be receiving this push, and it _might_ know).
Yeah this example was bad, but now since I've written it I've become
more convinced that it's the right way to go, just that I didn't explain
it well.
E.g. just now I did:
git push avar avar/some-branch:some-branch-wip
git push avar HEAD -f # I was on 'some-branch'
Because I'd regretted taking the "some-branch" name with some WIP code,
but didn't want to lose the work, so I wanted to swap.
It's also arbitrary and contrary to the distributed nature of git that
we're treating branches from other repos differently than the ones from
ours.
After all sometimes "other" is just the repo on my laptop or server. I
shouldn't need to jump through hoops to re-push stuff from my "other"
repo anymore than from the local repo.
Yes refs/remotes/* isn't guaranteed to be "other repo's branches" in the
same way our local refs/heads/* is, and this series bends over backwards
to check if someone's (ab)used it for that, but I think for the purposes
of the default UI we should treat it as "branches from other remotes" if
we find commit objects there.
So I think the *only* thing we should be checking for the purposes of
this DWYM feature is what local type of object (branch or tag) we find
on the LHS of the refspec. And if we're sure of its type push to
refs/{heads,tags}/* as appropriate.
I don't think it makes any sense as you suggest to move away from "guess
based on existing type" to breaking the dwimmery because we found the
branch in some other place. The user's pushing "newref" from an existing
branch, let's just assume the new thing is a branch, whether the source
is local or not.
> As to the entirety of the series,
>
> - I do not think this step 7, and its documentation in step 8, are
>particularly a good idea, in their current shape. Pushing tag
>objects to refs/tags/ is probably a good idea, but pushing a
>commit as local branch heads are necessarily not.
>
> - Step 6 is probably a good documentation on the cases in which we
>make and do not make guess on the unqualified push destination.
>
> - Step 5 and earlier looked like good changes.
>
> If we were to salvage some parts of step 7 (and step 8), we'd
> probably need fb7c2268 ("SQUASH???", 2018-10-29) to number all the
> placeholders in the printf format string.
Thanks. As noted will try to re-roll this soon.
RE: Failed stash caused untracked changes to be lost
Hi,
Thanks for the reply. Sorry I forgot the version number, completely slipped my
mind. At the time of writing the report it was Git ~ 2.17 I believe. All of our
software is updated centrally at my work, we have received an update since
writing this to 2.19.1. Unfortunately because of it being centrally controlled,
I couldn't update and try the latest version at the time (and now I can't go
back and check exactly what version I had).
I've never even looked at the git source or contributing before so I wouldn't
be sure where to start. If you (or someone) is happy to point me in the right
direction I'd be happy to take a look, I can't promise I'll be able to get
anything done in a timely manner (or at all)
Thanks
-Original Message-
From: Thomas Gummerer
Sent: 03 November 2018 15:35
To: Quinn, David
Cc: git@vger.kernel.org
Subject: Re: Failed stash caused untracked changes to be lost
Exercise Caution: This email is from an external source.
On 10/23, Quinn, David wrote:
>
> Issue: While running a git stash command including the '-u' flag to include
> untracked files, the command failed due to arguments in the incorrect order.
> After this untracked files the were present had been removed and permanently
> lost.
Thanks for your report (and sorry for the late reply)!
I believe this (somewhat) fixed in 833622a945 ("stash push: avoid printing
errors", 2018-03-19), which was first included in Git 2.18.
Your message doesn't state which version of Git you encountered the bug, but
I'm going to assume with something below 2.18 (For future reference, please
include the version of Git in bug reports, or even better test with the latest
version of Git, as the bug may have been fixed in the meantime).
Now I'm saying somewhat fixed above, because we still create an stash if a
pathspec that doesn't match any files is passed to the command, but then don't
remove anything from the working tree, which is a bit confusing.
I think the right solution here would be to error out early if we were given a
pathspec that doesn't match anything. I'll look into that, unless you're
interested in giving it a try? :)
> Environment: Windows 10, Powershell w/ PoshGit
>
>
> State before running command: 9 Modified files, 2 (new) untracked
> files
>
> Note: I only wanted to commit some of the modified files (essentially
> all the files/changes I wanted to commit were in one directory)
>
> Actual command run: git stash push -u -- Directory/To/Files/* -m "My Message"
>
> Returned:
>
> Saved working directory and index state WIP on [BranchName]: [Commit
> hash] [Commit Message]
> fatal: pathspec '-m' did not match any files
> error: unrecognized input
>
> State after Command ran: 9 Modifed files, 0 untracked files
>
>
> The command I should have ran should have been
>
> git stash push -u -m "My Message"? -- Directory/To/Files/*
>
>
> I have found the stash that was created by running this command:
>
> gitk --all $(git fsck --no-reflog | Select-String "(dangling
> commit )(.*)" | %{ $_.Line.Split(' ')[2] }) ?
> and searching for the commit number that was returned from the original
> (paritally failed??) stash command. However there is nothing in that stash.
> It is empty.
>
>
>
> I think that the fact my untracked files were lost is not correct
> behaviour and hence why I'm filing this bug report
>
>
>
>
>
> NOTICE: This message, and any attachments, are for the intended recipient(s)
> only, may contain information that is privileged, confidential and/or
> proprietary and subject to important terms and conditions available at
> E-Communication
> Disclaimer.
> If you are not the intended recipient, please delete this message. CME Group
> and its subsidiaries reserve the right to monitor all email communications
> that occur on CME Group information systems.
Re: [PATCH v2 2/5] pretty: allow showing specific trailers
On Mon, Nov 5, 2018 at 3:26 AM Anders Waldenborg wrote: > Eric Sunshine writes: > > Should the code tolerate a trailing colon? (Genuine question; it's > > easy to do and would be more user-friendly.) > > I would make sense to allow the trailing colon, it is easy enough to > just strip that away when reading the argument. > > However I'm not sure how that would fit together with the possibility to > later lifting it to a regexp, hard to strip a trailing colon from a > regexp in a generic way. Which is a good reason to think about these issues now, before being set in stone. > > What happens if 'key=...' is specified multiple times? > > My first thought was to simply disallow that. But that seemed hard to > fit into current model where errors just means don't expand. > > I would guess that most useful and intuitive to user would be to handle > multiple key arguments by showing any of those keys. Agreed. > > Thinking further on the last two points, should be a regular > > expression? > > It probably would make sense. I can see how the regexp '^.*-by$' would > be useful (but glob style matching would suffice in that case). > > Also handling multi-matching with an alternation group would be elegant > %(trailers:key="(A|B)"). Except for the fact that the parser would need to > understand some kind of quoting, which seems like an major undertaking. Maybe, maybe not. As long as we're careful not to paint ourselves into a corner, it might very well be okay to start with the current implementation of matching the full key as a literal string and (perhaps much) later introduce regex as an alternate way to specify the key. For instance, 'key=literal' and 'key=/regex/' can co-exist, and the extraction of the regex inside /.../ should not be especially difficult. > I guess having it as a regular exception would also mean that it needs > to get some way to cache the re so it is compiled once, and not for each > expansion. Yes, that's something I brought up a few years ago during a GSoC project; not regex specifically, but that this parsing of the format is happening repeatedly rather than just once. I had suggested to the GSoC student that the parsing could be done early, compiling the format expression into a "machine" which could be applied repeatedly. It's a larger job, of course, not necessarily something worth tackling for your current needs. > > If I understand correctly, this is making a copy of so that it > > will be NUL-terminated since the code added to trailer.c uses a simple > > strcasecmp() to match it. Would it make sense to avoid the copy by > > adding fields 'opts.filter_key' and 'opts.filter_key_len' and using > > strncasecmp() instead? (Genuine question; not necessarily a request > > for change.) > > I'm also not very happy about that copy. > Just using strncasecmp would cause it to be prefix match, no? Well, you could retain full key match by checking for NUL explicitly with something like this: !strncasecmp(tok.buf, opts->filter_key, opts->filter_key_len) && !tok.buf[opts->filter_key_len]
Re: [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function
Junio C Hamano writes: > I do not think "fundamental" is the best name for this, but I agree > that it would be useful to split the helpers into one that is > "constant across commits" and the other one that is "per commit". Any suggestions for a better name? standalone? simple? invariant? free?
Re: [PATCH v2 2/5] pretty: allow showing specific trailers
Eric Sunshine writes:
> Should the code tolerate a trailing colon? (Genuine question; it's
> easy to do and would be more user-friendly.)
I would make sense to allow the trailing colon, it is easy enough to
just strip that away when reading the argument.
However I'm not sure how that would fit together with the possibility to
later lifting it to a regexp, hard to strip a trailing colon from a
regexp in a generic way.
> What happens if 'key=...' is specified multiple times?
My first thought was to simply disallow that. But that seemed hard to
fit into current model where errors just means don't expand.
I would guess that most useful and intuitive to user would be to handle
multiple key arguments by showing any of those keys.
> Thinking further on the last two points, should be a regular expression?
It probably would make sense. I can see how the regexp '^.*-by$' would
be useful (but glob style matching would suffice in that case).
Also handling multi-matching with an alternation group would be elegant
%(trailers:key="(A|B)"). Except for the fact that the parser would need to
understand some kind of quoting, which seems like an major undertaking.
I guess having it as a regular exception would also mean that it needs
to get some way to cache the re so it is compiled once, and not for each
expansion.
>
>> + free(opts.filter_key);
>
> If I understand correctly, this is making a copy of so that it
> will be NUL-terminated since the code added to trailer.c uses a simple
> strcasecmp() to match it. Would it make sense to avoid the copy by
> adding fields 'opts.filter_key' and 'opts.filter_key_len' and using
> strncasecmp() instead? (Genuine question; not necessarily a request
> for change.)
I'm also not very happy about that copy.
Just using strncasecmp would cause it to be prefix match, no?
But if changing matching semantics to handle multiple key options to
something else I'm thinking opts.filter_key would be replaced with a
opts.filter callback function, and that part would need to be rewritten
anyway.
>
>> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
>> @@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '
>> +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
>> + git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual &&
>> + {
>> + echo "Acked-by: A U Thor " &&
>> + echo
>> + } >expect &&
>> + test_cmp expect actual
>> +'
>
> I guess these new tests are modeled after one or two existing tests
> which use a series of 'echo' statements
I guess I could change it to "--pretty=format:%(trailers:key=Acked-by)"
to get separator semantics and avoid that extra blank line, making it
simpler.
