Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
Ævar Arnfjörð Bjarmason writes: > * We error out in the Makefile if you're still saying >GETTEXT_POISON=YesPlease. I expected this would be irritating, but it turns out it is worse than mere irritation but is a severe hinderance to affect my performance, as I (and my bots) keep building and testing different versions of Git under different configurations. I know it was done to help those who only ever build a single track at a time and mostly moving forward, but I'm very much tempted to remove this part, perhaps demote it to a warning of some sort. > ifdef GETTEXT_POISON > - BASIC_CFLAGS += -DGETTEXT_POISON > +$(error The GETTEXT_POISON option has been removed in favor of runtime > GIT_TEST_GETTEXT_POISON. See t/README!) > endif -- >8 -- Makefile: ease dynamic-gettext-poison transition Earlier we made the entire build to fail when GETTEXT_POISON=Yes is given to make, to notify those who did not notice that text poisoning is now a runtime behaviour. It turns out that this too irritating for those who need to build and test different versions of Git that cross the boundary between history with and without this topic to switch between two environment variables. Demote the error to a warning, so that you can say something like make GETTEXT_POISON=Yes GIT_TEST_GETTEXT_POISON test during the transition period, without having to worry about whether exact version you are testing has or does not have this topic. Signed-off-by: Junio C Hamano --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index f3a9995e50..6b492f44a6 100644 --- a/Makefile +++ b/Makefile @@ -1447,7 +1447,7 @@ ifdef NO_SYMLINK_HEAD BASIC_CFLAGS += -DNO_SYMLINK_HEAD endif ifdef GETTEXT_POISON -$(error The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!) +$(warning The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!) endif ifdef NO_GETTEXT BASIC_CFLAGS += -DNO_GETTEXT
Re: [PATCH 1/1] http: add support selecting http version
"Force Charlie via GitGitGadget" writes:
> From: Force Charlie
>
> Signed-off-by: Force Charlie
> ---
> http.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/http.c b/http.c
> index 3dc8c560d6..99cb04faba 100644
> --- a/http.c
> +++ b/http.c
> @@ -48,6 +48,7 @@ char curl_errorstr[CURL_ERROR_SIZE];
>
> static int curl_ssl_verify = -1;
> static int curl_ssl_try;
> +static int curl_http_version = 11;
Is there any reason that we need to have this variable's value to be
"int"? I _think_ in this patch, the variable is used to choose
between the default and "HTTP/2", and I do not think the updated
code can choose any other new value that may be supported by an even
newer cURL library without further update, i.e. we'd need a variant of
"if the configuration asks HTTP/2 then use CURLOPT_HTTP_VERSION with
CURL_HTTP_VERSION_2" for the new choice.
So I'd think it would not add much value to force end users use a
rather cryptic "20" (vs "11") to choose between "2" and "1.1". Why
not use spell it out, e.g. using the official name of the protocol
"HTTP/2" (vs "HTTP/1.1"), with a "const char *" instead?
The new configuration variable and the possible values it can take
must be documented, of course. I think it would make the description
far less embarrassing if we say "HTTP/2" etc. rather than "20",
"11", etc.
> @@ -284,6 +285,10 @@ static void process_curl_messages(void)
>
> static int http_options(const char *var, const char *value, void *cb)
> {
> + if (!strcmp("http.version",var)) {
> + curl_http_version=git_config_int(var,value);
STYLE. Missing SP after comma, and around assignment.
> + return 0;
> + }
> if (!strcmp("http.sslverify", var)) {
> curl_ssl_verify = git_config_bool(var, value);
> return 0;
> @@ -806,6 +811,13 @@ static CURL *get_curl_handle(void)
> curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 2);
> }
>
> +#if LIBCURL_VERSION_NUM >= 0x073100
> + if(curl_http_version == 20){
STYLE. Missing SP before opening paren and after closing paren.
> + /* CURL Enable HTTP2*/
STYLE. Missing SP before closing asterisk-slash.
> + curl_easy_setopt(result,
> CURLOPT_HTTP_VERSION,CURL_HTTP_VERSION_2);
> + }
> +#endif
Shouldn't this block also handle the other values, e.g. "11"?
I _think_ the curl_http_version variable (be it an deci-int, or a
const char *) should be initialized to a value that you can use to
notice that the configuration did not specify any, and then this
part should become more like
if (curl_http_version &&
!get_curl_http_version_opt(curl_http_version, ))
curl_easy_setopt(result, CURL_HTTP_VERSION, opt);
with a helper function like this:
static int get_curl_http_version_opt(const char *version_string, long *opt)
{
int i;
static struct {
const char *name;
lnog opt_token;
} choice[] = {
{ "HTTP/1.1", CURL_HTTP_VERSION_1_1 },
{ "HTTP/2", CURL_HTTP_VERSION_2 },
};
for (i = 0; i < ARRAY_SIZE(choice); i++) {
if (!strcmp(version_string, choice[i].name)) {
*opt = choice[i].opt_token;
return 0;
}
}
return -1; /* not found */
}
which would make it trivial to support new values later.
> #if LIBCURL_VERSION_NUM >= 0x070907
> curl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
> #endif
Re: [PATCH] branch: make --show-current use already resolved HEAD
Rafael Ascensão writes:
> print_current_branch_name() tries to resolve HEAD and die() when it
> doesn't resolve it successfully. But the conditions being tested are
> always unreachable because early in branch:cmd_branch() the same logic
> is performed.
>
> Eliminate the duplicate and unreachable code, and update the current
> logic to the more reliable check for the detached head.
Nice.
> I still think the mention about scripting should be removed from the
> original commit message, leaving it open to being taught other tricks
> like --verbose that aren't necessarily script-friendly.
I'd prefer to see scriptors avoid using "git branch", too.
Unlike end-user facing documentation where we promise "we do X and
will continue to do so because of Y" to the readers, the log message
is primarily for recording the original motivation of the change, so
that we can later learn "we did X back then because we thought Y".
When we want to revise X, we revisit if the reason Y is still valid.
So in that sense, the door to "break" the scriptability is still
open.
> But the main goal of this patch is to just bring some attention to this,
> as I mentioned it in a previous thread but it got lost.
This idea of yours seems to lead to a better implementation, and
indeed "got lost" is a good way to describe what happened---I do not
recall seeing it, for example. Thanks for bringing it back.
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 46f91dc06d..1c51d0a8ca 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -38,6 +38,7 @@ static const char * const builtin_branch_usage[] = {
>
> static const char *head;
> static struct object_id head_oid;
> +static int head_flags = 0;
You've eliminated the "now unnecessary" helper and do everything
inside cmd_branch(), so perhaps this can be made function local, no?
> @@ -668,10 +654,10 @@ int cmd_branch(int argc, const char **argv, const char
> *prefix)
>
> track = git_branch_track;
>
> - head = resolve_refdup("HEAD", 0, _oid, NULL);
> + head = resolve_refdup("HEAD", 0, _oid, _flags);
> if (!head)
> die(_("Failed to resolve HEAD as a valid ref."));
> - if (!strcmp(head, "HEAD"))
> + if (!(head_flags & REF_ISSYMREF))
> filter.detached = 1;
Nice to see we can reuse the resolve_refdup() we already have.
> else if (!skip_prefix(head, "refs/heads/", ))
> die(_("HEAD not found below refs/heads!"));
> @@ -716,7 +702,8 @@ int cmd_branch(int argc, const char **argv, const char
> *prefix)
> die(_("branch name required"));
> return delete_branches(argc, argv, delete > 1, filter.kind,
> quiet);
> } else if (show_current) {
> - print_current_branch_name();
> + if (!filter.detached)
> + puts(head);
Ah, I wondered why we do not have to skip-prefix, but it is already
done for us when we validated that an attached HEAD points at a
local branch. Good.
> return 0;
> } else if (list) {
> /* git branch --local also shows HEAD when it is detached */
Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert
Duy Nguyen writes: > There is still one thing to settle. "revert -m1" could produce > something like this > > This reverts commit , reversing > changes made to . I do not think it is relevant, with or without multiple parents, to even attempt to read this message. The description is not meant to be machine readable/parseable, but is meant to be updated to describe the reason why the reversion was made for human readers. Spending any cycle to attempt interpreting it by machines will give a wrong signal to encourage people not to touch it. Instead we should actively encourage people to take that as the beginning of their description. I even suspect that an update to that message to read something like these "This reverts commit because FILL IN THE REASONS HERE" "This reverts commit , reversing changes made to , because FILL IN THE REASONS HERE" would be a good idea. It of course is orthogonal to the topic of introducing a new footer to record the "what happened" (without the "why") in a machine-readable way.
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Jeff King writes: > On Wed, Nov 07, 2018 at 10:36:52PM +0100, Johannes Sixt wrote: > > All that said, if we're just interested in allowing this for config, > then we already have such a wrapper function: git_config_pathname(). > > So I don't think it's a big deal to implement it in any of these ways. > It's much more important to get the syntax right, because that's > user-facing and will be with us forever. All of us are on the same page after seeing the clarification by Dscho, it seems. I came to pretty much the same conclusion this morning before reading this subthread. Outside config values, the callers of expand_user_path() only feed "~/.git$constant", and they are all about "personal customization" that do not want to be shared with other users of the same installation, so "relative to runtime prefix" feature would not be wanted. But we do not know about new caller's needs. For now I am OK to have it in expand_user_path(), possibly renaming the function if people feel it is needed (I don't). Between ~ and $VARIABLE_LOOKING_THINGS, I do not have a strong preference either way, but I am getting an impression that the latter is more generally favoured in the discussion?
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Ramsay Jones writes: >> The cute thing is: your absolute paths would not be moved because we are >> talking about Windows. Therefore your absolute paths would not start with >> a forward slash. > > Ah, sorry, I must have misunderstood a comment in your cover letter: > > The reason is this: something like this (make paths specified e.g. via > http.sslCAInfo relative to the runtime prefix) is potentially useful > also in the non-Windows context, as long as Git was built with the > runtime prefix feature. > > ... so I thought you meant to add this code for POSIX systems as well. > > My mistake. :( Well, I do not think you should feel so bad about it, as you are not alone. It wasn't clear to me either that it was to introduce a new syntax that users would not have otherwise used before (i.e. avoids negatively affecting existing settings---because the users would have used a path that begins with a backslash if they meant "full path down from the top of the current drive") to mean a new thing (i.e. "relative to the runtime prefix") that the patch wanted to do. If that is the motivation, then it does make sense to extend this function, and possibly rename it, like this patch does, as we would want this new syntax available in anywhere we take a pathname to an untracked thing. And for such a pathname, we should be consistently using expand_user_path() *anyway* even without this new "now we know how to spell 'relative to the runtime prefix'" feature. So I agree with the patch that the issue it wants to address is worth addressing, and the function to enhance is this one. I am still unsure about the solution, though. I suspect that any build that uses runtime prefix would want access to this feature, regardless of the platform. The new syntax that is "a pathname that begins with a slash is not an absolute path but is relative to the runtime prefix" cannot avoid ambiguity with users' existing settings on platforms other than Windows, which means this feature cannot be made platform neutral in its posted form. The documentation must say "On windows, the way to spell runtime prefix relative paths is this; on macs, you must spell it differently like this." etc. Which is rather unfortunate. Perhaps we need to make an effort to find a syntax that is universally unambiguous and use it consistently across platforms? I am tempted to say "///" might also be such a way, even in the POSIX world, but am not brave enough to do so, as I suspect that may have a fallout in the Windows world X-<. An earlier suggestion by Duy in [*1*] to pretend as if we take $VARIABLE and define special variables might be a better direction. Are there security implications if we started allowing references to environment varibables in strings we pass expand_user_path()? If we cannot convince ourselves that it is safe to allow access to any and all environment variables, then we probably can start by specific "pseudo variables" under our control, like GIT_EXEC_PATH and GIT_INSTALL_ROOT, that do not even have to be tied to environment variables, perhaps with further restriction to allow it only at the beginning of the string, or something like that, if necessary. [References] *1*
Re: [PATCH v3 1/2] range-diff doc: add a section about output stability
Stephen & Linda Smith writes: >> +This is particularly true when passing in diff options. Currently some >> +options like `--stat` can as an emergent effect produce output that's > > "`--stat` can as an emergent": I read that for times to decided it was > correct > grammar. Should it be reworded to read better? Just a nit. A pair of comma around "as an ... effect" would make it a bit more readable. It also took me four reads before I convinced myself that the original wants to say "Some options may just do whatever they happen to do that result in pretty useless output". > >> +quite useless in the context of `range-diff`. Future versions of >> +`range-diff` may learn to interpret such options in a manner specifc >> +to `range-diff` (e.g. for `--stat` summarizing how the diffstat >> +changed).
Re: What's cooking in git.git (Nov 2018, #03; Wed, 7)
Duy Nguyen writes:
> On Wed, Nov 7, 2018 at 2:09 PM SZEDER Gábor wrote:
>>
>> On Wed, Nov 07, 2018 at 06:41:45PM +0900, Junio C Hamano wrote:
>> > * nd/i18n (2018-11-06) 16 commits
>> > - fsck: mark strings for translation
>> > - fsck: reduce word legos to help i18n
>> > ...
>> > More _("i18n") markings.
>>
>> When this patch is merged into 'pu' all four tests added to
>> 't1450-fsck.sh' in b29759d89a (fsck: check HEAD and reflog from other
>> worktrees, 2018-10-21) as part of 'nd/per-worktree-ref-iteration'
>> below fail when run with GETTEXT_POISON=y. The test suite passes in
>> both of these topics on their own, even with GETTEXT_POISON, it's
>> their merge that is somehow problematic.
>
> Not surprising. The i18n series makes fsck output localized strings
> and without updating grep to test_i18ngrep, new tests will fail. If
> 'pu' was passing before, I'm ok with just ejecting this series for
> now. Then I wait for the other to land, rebase, fixup and resubmit.
Let me first see if I can come up with a merge-fix that can be
carried around during the time this topic cooks, before talking
about dropping and reattempting the series.
For a change like this, a time-window in which the codebase is
quiescent enough may never come, and because the changes go all over
the place, mostly but not entirely repetitive, it costs a lot, not
just to write but also to review them.
Re: What's cooking in git.git (Nov 2018, #03; Wed, 7)
Ævar Arnfjörð Bjarmason writes: > On Wed, Nov 07 2018, Junio C Hamano wrote: > >> * ab/range-diff-no-patch (2018-11-07) 1 commit >> - range-diff: add a --no-patch option to show a summary >> >> "range-diff" learns the "--no-patch" option, which can be used to >> get a high-level overview without the actual line-by-line patch >> difference shown. >> >> Will merge to 'next'. > > Per <20181107122202.1813-1-ava...@gmail.com> it turns out this whole > thing should have been a bugfix instead, sent a v2. Thanks.
What's cooking in git.git (Nov 2018, #03; Wed, 7)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * en/merge-cleanup-more (2018-10-18) 2 commits (merged to 'next' on 2018-10-26 at c706319c26) + merge-recursive: avoid showing conflicts with merge branch before HEAD + merge-recursive: improve auto-merging messages with path collisions (this branch is used by en/merge-path-collision.) Further clean-up of merge-recursive machinery. * jc/http-curlver-warnings (2018-10-26) 1 commit (merged to 'next' on 2018-10-26 at 870e125cec) + http: give curl version warnings consistently (this branch uses js/mingw-http-ssl; is tangled with nd/config-split.) Warning message fix. * js/mingw-http-ssl (2018-10-26) 3 commits (merged to 'next' on 2018-10-26 at 318e82e101) + http: when using Secure Channel, ignore sslCAInfo by default + http: add support for disabling SSL revocation checks in cURL + http: add support for selecting SSL backends at runtime (this branch is used by jc/http-curlver-warnings and nd/config-split.) On platforms with recent cURL library, http.sslBackend configuration variable can be used to choose a different SSL backend at runtime. The Windows port uses this mechanism to switch between OpenSSL and Secure Channel while talking over the HTTPS protocol. * js/mingw-ns-filetime (2018-10-24) 3 commits (merged to 'next' on 2018-10-29 at 4563a0d9d0) + mingw: implement nanosecond-precision file times + mingw: replace MSVCRT's fstat() with a Win32-based implementation + mingw: factor out code to set stat() data Windows port learned to use nano-second resolution file timestamps. * js/remote-archive-dwimfix (2018-10-26) 1 commit (merged to 'next' on 2018-10-26 at f5bf6946bd) + archive: initialize archivers earlier The logic to determine the archive type "git archive" uses did not correctly kick in for "git archive --remote", which has been corrected. * js/shallow-and-fetch-prune (2018-10-25) 3 commits (merged to 'next' on 2018-10-26 at 93b7196560) + repack -ad: prune the list of shallow commits + shallow: offer to prune only non-existing entries + repack: point out a bug handling stale shallow info "git repack" in a shallow clone did not correctly update the shallow points in the repository, leading to a repository that does not pass fsck. * jt/upload-pack-v2-fix-shallow (2018-10-19) 3 commits (merged to 'next' on 2018-10-29 at d9010b3c7b) + upload-pack: clear flags before each v2 request + upload-pack: make want_obj not global + upload-pack: make have_obj not global "git fetch" over protocol v2 into a shallow repository failed to fetch full history behind a new tip of history that was diverged before the cut-off point of the history that was previously fetched shallowly. * jw/send-email-no-auth (2018-10-23) 1 commit (merged to 'next' on 2018-10-29 at a3fbbdb889) + send-email: explicitly disable authentication "git send-email" learned to disable SMTP authentication via the "--smtp-auth=none" option, even when the smtp username is given (which turns the authentication on by default). * md/exclude-promisor-objects-fix (2018-10-23) 2 commits (merged to 'next' on 2018-10-29 at fb36a5dcbe) + exclude-promisor-objects: declare when option is allowed + Documentation/git-log.txt: do not show --exclude-promisor-objects Operations on promisor objects make sense in the context of only a small subset of the commands that internally use the revisions machinery, but the "--exclude-promisor-objects" option were taken and led to nonsense results by commands like "log", to which it didn't make much sense. This has been corrected. * mg/gpg-fingerprint (2018-10-23) 3 commits (merged to 'next' on 2018-10-26 at 1e219cb754) + gpg-interface.c: obtain primary key fingerprint as well + gpg-interface.c: support getting key fingerprint via %GF format + gpg-interface.c: use flags to determine key/signer info presence (this branch uses mg/gpg-parse-tighten; is tangled with jc/gpg-cocci-preincr.) New "--pretty=format:" placeholders %GF and %GP that show the GPG key fingerprints have been invented. * mg/gpg-parse-tighten (2018-10-22) 1 commit (merged to 'next' on 2018-10-26 at efdec77193) + gpg-interface.c: detect and reject multiple signatures on commits (this branch is used by jc/gpg-cocci-preincr and mg/gpg-fingerprint.) Detect and reject a signature block that has more than one GPG signature. * nd/completion-negation (2018-10-22) 1 commit (merged to 'next' on 2018-10-29 at 87e73b0c72) + completion: fix __gitcomp_builtin no longer
Re: [PATCH 1/1] Windows: force-recompile git.res for differing architectures
Johannes Sixt writes: > Am 07.11.18 um 02:32 schrieb Junio C Hamano: >> Johannes Sixt writes: >>> On Linux, when I recompile for a different architecture, CFLAGS would >>> change, so I would have thought that GIT-CFLAGS were the natural >>> choice for a dependency. Don't they change in this case on Windows, >>> too? >> >> Depending on GIT-CFLAGS would have a better chance of being safe, I >> guess, even though it can at times be overly safe, than GIT-PREFIX, >> I suspect. As a single user target in Makefile, which is only used >> by Dscho, who intends to stick to /mingw(32|64)/ convention til the >> end of time, I think the posted patch is OK, though. > > I think that it's not only Dscho who uses the target (my build > environment, for example, is different from Dscho's and compiles > git.res, too). But since the patch helps him most and doesn't hurt > others, it is good to go. No objection from my side. Yeah, that was phrased poorly. What I meant was by "only by Dscho" was "only by those who share the convention that GIT-PREFIX is changed if and only if targetting a different arch". Anyway, I just wanted to say that GIT-PREFIX may not be precise enough but would give sufficient signal; GIT-CFLAGS may be a more cautious choice, but it may change a bit too often ;-).
Re: [PATCH v2 1/1] pack-objects: ignore ambiguous object warnings
Junio C Hamano writes:
> The fix here is similar to 4c30d50 "rev-list: disable object/refname
> ambiguity check with --stdin". While the get_object_list() method
> reads the objects from stdin, turn warn_on_object_refname_ambiguity
> flag (which is usually true) to false. Just for code hygiene, save
> away the original at the beginning and restore it once we are done.
I actually think this is a bit too broad. The calling process of
this program does know that it is feeding list of raw object names
(prefixed with ^ for negative ones), but the codepath this patch
touches does not know who is calling it with what. It would be
safer to introduce a mechanism for the caller to tell this codepath
not to bother checking refnames, as it knows it is feeding the raw
object names and not refnames.
After all, you can feed things like "refs/heads/master" from the
standard input of "git pack-objects --revs", no?
> Helped-by: Jeff King
> Signed-off-by: Derrick Stolee
> Signed-off-by: Junio C Hamano
> ---
> builtin/pack-objects.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index d1144a8f7e..f703e6df9b 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2988,6 +2988,7 @@ static void get_object_list(int ac, const char **av)
> struct rev_info revs;
> char line[1000];
> int flags = 0;
> + int save_warning;
>
> init_revisions(, NULL);
> save_commit_buffer = 0;
> @@ -2996,6 +2997,9 @@ static void get_object_list(int ac, const char **av)
> /* make sure shallows are read */
> is_repository_shallow(the_repository);
>
> + save_warning = warn_on_object_refname_ambiguity;
> + warn_on_object_refname_ambiguity = 0;
> +
> while (fgets(line, sizeof(line), stdin) != NULL) {
> int len = strlen(line);
> if (len && line[len - 1] == '\n')
> @@ -3022,6 +3026,8 @@ static void get_object_list(int ac, const char **av)
> die(_("bad revision '%s'"), line);
> }
>
> + warn_on_object_refname_ambiguity = save_warning;
> +
> if (use_bitmap_index && !get_object_list_from_bitmap())
> return;
Re: [PATCH 2/3] approxidate: handle pending number for "specials"
Jeff King writes:
> On Tue, Nov 06, 2018 at 04:48:28PM -0800, Carlo Arenas wrote:
>
> I think date_yesterday() is the only one of those special functions that
> gets called like this. Here's what I think we should do to fix it (this
> can go right on top of jk/misc-unused-fixes, which is already in next).
Thanks, both. I think the patch makes sense.
> -- >8 --
> Subject: [PATCH] approxidate: fix NULL dereference in date_time()
>
> When we see a time like "noon", we pass "12" to our date_time() helper,
> which sets the hour to 12pm. If the current time is before noon, then we
> wrap around to yesterday using date_yesterday(). But unlike the normal
> calls to date_yesterday() from approxidate_alpha(), we pass a NULL "num"
> parameter. Since c27cc94fad (approxidate: handle pending number for
> "specials", 2018-11-02), that causes a segfault.
>
> One way to fix this is by checking for NULL. But arguably date_time() is
> abusing our helper by passing NULL in the first place (and this is the
> only case where one of these "special" parsers is used this way). So
> instead, let's have it just do the 1-day subtraction itself. It's still
> just a one-liner due to our update_tm() helper.
>
> Note that the test added here is a little funny, as we say "10am noon",
> which makes the "10am" seem pointless. But this bug can only be
> triggered when it the currently-parsed hour is before the special time.
> The latest special time is "tea" at 1700, but t0006 uses a hard-coded
> TEST_DATE_NOW of 1900. We could reset TEST_DATE_NOW, but that may lead
> to confusion in other tests. Just saying "10am noon" makes this test
> self-contained.
>
> Reported-by: Carlo Arenas
> Signed-off-by: Jeff King
> ---
> date.c | 2 +-
> t/t0006-date.sh | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/date.c b/date.c
> index 7adce327a3..9bc15df6f9 100644
> --- a/date.c
> +++ b/date.c
> @@ -929,7 +929,7 @@ static void date_yesterday(struct tm *tm, struct tm *now,
> int *num)
> static void date_time(struct tm *tm, struct tm *now, int hour)
> {
> if (tm->tm_hour < hour)
> - date_yesterday(tm, now, NULL);
> + update_tm(tm, now, 24*60*60);
> tm->tm_hour = hour;
> tm->tm_min = 0;
> tm->tm_sec = 0;
> diff --git a/t/t0006-date.sh b/t/t0006-date.sh
> index b7ea5fbc36..ffb2975e48 100755
> --- a/t/t0006-date.sh
> +++ b/t/t0006-date.sh
> @@ -114,6 +114,7 @@ check_approxidate '15:00' '2009-08-30 15:00:00'
> check_approxidate 'noon today' '2009-08-30 12:00:00'
> check_approxidate 'noon yesterday' '2009-08-29 12:00:00'
> check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00'
> +check_approxidate '10am noon' '2009-08-29 12:00:00'
>
> check_approxidate 'last tuesday' '2009-08-25 19:20:00'
> check_approxidate 'July 5th' '2009-07-05 19:20:00'
Re: [PATCH v2 1/1] pack-objects: ignore ambiguous object warnings
Jeff King writes:
> So we'd never expect to see anything except "1" in our save_warning
> variable. Doing a save/restore is just about code hygiene and
> maintainability.
Here is what I plan to queue. Thanks, both.
-- >8 --
From: Derrick Stolee
Date: Tue, 6 Nov 2018 12:34:47 -0800
Subject: [PATCH] pack-objects: ignore ambiguous object warnings
A git push process runs several processes during its run, but one
includes git send-pack which calls git pack-objects and passes
the known have/wants into stdin using object ids. However, the
default setting for core.warnAmbiguousRefs requires git pack-objects
to check for ref names matching the ref_rev_parse_rules array in
refs.c. This means that every object is triggering at least six
"file exists?" queries. When there are a lot of refs, this can
add up significantly! I observed a simple push spending three
seconds checking these paths.
The fix here is similar to 4c30d50 "rev-list: disable object/refname
ambiguity check with --stdin". While the get_object_list() method
reads the objects from stdin, turn warn_on_object_refname_ambiguity
flag (which is usually true) to false. Just for code hygiene, save
away the original at the beginning and restore it once we are done.
Helped-by: Jeff King
Signed-off-by: Derrick Stolee
Signed-off-by: Junio C Hamano
---
builtin/pack-objects.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d1144a8f7e..f703e6df9b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2988,6 +2988,7 @@ static void get_object_list(int ac, const char **av)
struct rev_info revs;
char line[1000];
int flags = 0;
+ int save_warning;
init_revisions(, NULL);
save_commit_buffer = 0;
@@ -2996,6 +2997,9 @@ static void get_object_list(int ac, const char **av)
/* make sure shallows are read */
is_repository_shallow(the_repository);
+ save_warning = warn_on_object_refname_ambiguity;
+ warn_on_object_refname_ambiguity = 0;
+
while (fgets(line, sizeof(line), stdin) != NULL) {
int len = strlen(line);
if (len && line[len - 1] == '\n')
@@ -3022,6 +3026,8 @@ static void get_object_list(int ac, const char **av)
die(_("bad revision '%s'"), line);
}
+ warn_on_object_refname_ambiguity = save_warning;
+
if (use_bitmap_index && !get_object_list_from_bitmap())
return;
--
2.19.1-856-g8858448bb4
Re: [PATCH 1/1] Windows: force-recompile git.res for differing architectures
Johannes Sixt writes: > Am 06.11.18 um 15:55 schrieb Johannes Schindelin via GitGitGadget: >> From: Johannes Schindelin >> >> When git.rc is compiled into git.res, the result is actually dependent >> on the architecture. That is, you cannot simply link a 32-bit git.res >> into a 64-bit git.exe. >> >> Therefore, to allow 32-bit and 64-bit builds in the same directory, we >> let git.res depend on GIT-PREFIX so that it gets recompiled when >> compiling for a different architecture (this works because the exec path >> changes based on the architecture: /mingw32/libexec/git-core for 32-bit >> and /mingw64/libexec/git-core for 64-bit). > > On Linux, when I recompile for a different architecture, CFLAGS would > change, so I would have thought that GIT-CFLAGS were the natural > choice for a dependency. Don't they change in this case on Windows, > too? Depending on GIT-CFLAGS would have a better chance of being safe, I guess, even though it can at times be overly safe, than GIT-PREFIX, I suspect. As a single user target in Makefile, which is only used by Dscho, who intends to stick to /mingw(32|64)/ convention til the end of time, I think the posted patch is OK, though.
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Ramsay Jones writes:
> On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin
>>
>> On Windows, an absolute POSIX path needs to be turned into a Windows
>> one.
>>
>> Signed-off-by: Johannes Schindelin
>> ---
>> path.c | 5 +
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/path.c b/path.c
>> index 34f0f98349..a72abf0e1f 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -11,6 +11,7 @@
>> #include "path.h"
>> #include "packfile.h"
>> #include "object-store.h"
>> +#include "exec-cmd.h"
>>
>> static int get_st_mode_bits(const char *path, int *mode)
>> {
>> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
>>
>> if (path == NULL)
>> goto return_null;
>> +#ifdef __MINGW32__
>> +if (path[0] == '/')
>> +return system_path(path + 1);
>> +#endif
>
> Hmm, this doesn't quite fit with the intended use of this
> function! ;-) (even on windows!)
>
> I haven't looked very deeply, but doesn't this affect all
> absolute paths in the config read by git_config_pathname(),
> along with all 'included config' files?
I think so. I have not thought things through to say if replacing a
"full path in the current drive" with system_path() is a sensible
thing to do in the first place, but I am getting the impression from
review comments that it probably is not.
> I am pretty sure that I would not want the absolute paths
> in my config file(s) magically 'moved' depending on whether
> git has been compiled with 'runtime prefix' support or not!
In any case, the helper is about expanding ~/foo and ~who/foo to
absolute paths, without touching other paths, so it is a wrong
function to implement it in, even if the motivation were sensible.
Re: [PATCH v2] range-diff: add a --no-patch option to show a summary
Ævar Arnfjörð Bjarmason writes:
> diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> index f01a0be851..05d1f6b6b6 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 no_patch = 0;
> 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_F('s', "no-patch", _patch,
> + N_("show patch output"), PARSE_OPT_NONEG),
As OPT_BOOL("patch") natively takes "--no-patch" to flip the bool
off, an int variable "patch" that is initialized to 1 would make it
more readable by avoiding double negation !no_patch like the one we
see below. I guess the reason behind the contortion you wanted to
give the synonym --silent to it?
> @@ -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, !no_patch, );
> strbuf_release();
> strbuf_release();
> @@ -7,6 +7,7 @@
>
> int show_range_diff(const char *range1, const char *range2,
> int creation_factor, int dual_color,
> + int patch,
> struct diff_options *diffopt);
Other than that small "Huh?", the code looks good to me.
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 6aae364171..27e071650b 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -122,6 +122,26 @@ test_expect_success 'changed commit' '
> test_cmp expected actual
> '
>
> +test_expect_success 'changed commit -p & --patch' '
> + git range-diff --no-color -p topic...changed >actual &&
> + test_cmp expected actual &&
> + git range-diff --no-color --patch topic...changed >actual &&
> + test_cmp expected actual
This makes sure that -p and --patch produces the same output as the
default case? I am not sure who in the parseopt API groks the
single letter "-p" in this case offhand. Care to explain how?
The other side of the test to see -s/--no-patch we see below also
makes sense.
> +test_expect_success 'changed commit -s & --no-patch' '
> +...
> + cat >expected <<-EOF &&
Quote EOF to allow readers skim the contents without looking for and
worrying about $substitutions in there, unless there are tons of
offending code in the same script already in which case we should
leave the clean-up outside this primary change.
Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert
Jeff King writes: > Could we help the reading scripts by normalizing old and new output via > interpret-trailers, %(trailers), etc? > > I think "(cherry picked from ...)" is already considered a trailer by > the trailer code. ;-) Great minds think alike, I guess. I think it is a great idea to ask interpret-trailers to help us here.
Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert
Nguyễn Thái Ngọc Duy writes:
> OK here is a less constroversal attempt to add new trailers. Instead
> of changing the default behavior (which could be done incrementally
> later), this patch simply adds a new option --append-trailer to revert
> and cherry-pick.
I almost agree, except that the the textual description in a revert
and the funny-looking trailer-wannabe in a cherry-pick are two
fundamentally different things, and adding duplicate to the latter
does not make much sense, while keeping both does make sense for the
former.
> PS. maybe --append-trailer is too generic? We have --signoff which is
> also a trailer. But that one carries more weights than just "machine
> generated tags".
> + if (opts->append_trailer) {
> + strbuf_addstr(, "\n");
> + if (parent_id != -1)
> + strbuf_addf(, "Reverts: %s~%d\n",
> + oid_to_hex(>object.oid),
> + parent_id);
> + else
> + strbuf_addf(, "Reverts: %s\n",
> + oid_to_hex(>object.oid));
> + }
Makes sense, I guess.
> @@ -1780,14 +1792,28 @@ static int do_pick_commit(enum todo_command command,
> struct commit *commit,
> if (find_commit_subject(msg.message, ))
> strbuf_addstr(, p);
>
> - if (opts->record_origin) {
> + if (opts->record_origin || opts->append_trailer) {
> strbuf_complete_line();
> if (!has_conforming_footer(, NULL, 0))
> strbuf_addch(, '\n');
> + }
> +
> + if (opts->record_origin) {
> strbuf_addstr(, cherry_picked_prefix);
> strbuf_addstr(, oid_to_hex(>object.oid));
> strbuf_addstr(, ")\n");
> }
> + if (opts->append_trailer) {
These should be either-or, I would think, as adding exactly the same
piece of information in two different machine-readable form does not
make much sense. The command line parser may even want to make sure
that both are not given at the same time.
Alternatively, we can keep using -x as "we are going to record where
the change was cherry-picked from" and use .record_origin to store
that bit, and use the new .append_trailer bit as "if we are recording
the origin, in which format between the old and the new do we do
so?" bit.
I think the latter makes more sense, at least to me.
> + if (opts->record_origin)
> + strbuf_addstr(, "\n");
> + if (parent_id != -1)
> + strbuf_addf(, "Cherry-picked-from:
> %s~%d\n",
> + oid_to_hex(>object.oid),
> + parent_id);
> + else
> + strbuf_addf(, "Cherry-picked-from: %s\n",
> + oid_to_hex(>object.oid));
> + }
Re: What exactly is a "initial checkout"
Christian Halstrick writes: > I am trying to teach JGit [1] to behave like native git regarding some > corner cases during "git checkout". I am reading the "git read-tree" > documentation and I am not sure about the case [2]. Git should behave > differently during a normal checkout than when you are doing a > "initial checkout". When you are starting from commit H and checking out a different commit M, and when a path in the index does not match what is recorded in commit H, usually Git tries to keep the state of the path you have in your index as a "local change", as long as the data recorded for the path is the same between H and M. A path in the index that matches what is recorded in commit H and with different data recorded for it in commit M gets M's version in the index and the working file is updated to match (but it requires that either the working tree file is missing, or the working tree version matches what is in the original index, to avoid data loss). But imagine you have just cloned and are trying to finish that process. What Git has done so far would include creating an empty repository, populating the object database and pointing branches at various commits. HEAD now points at the branch (usually 'master'), the index file does not exist (you haven't checked out anything), and we want to populate the index and the working tree files to match what is recorded in HEAD. We do so by starting from commit HEAD and checking out commit HEAD. This situation presents conflicting goals to the above "keep the local change" rule. To the rule, this situation looks as if you removed each and every path from the index (as the index hasn't been populated yet---in fact, the index file does not even exist yet in this state), but the data recorded for each path are the same between commit H and commit M (as H==M==HEAD in this case), so "keep the local change" rule would leave the index and the working tree empty X-<. That is rescued by the "initial checkout behaves differently and forces the index and the working tree match what is recorded in commit M" exception. It probably should be obvious to the readers by now that the absense of .git/index is used as the clue for this exception to kick in from the above use case. And that is exactly the condition that is checked by read-cache.c::is_index_unborn().
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 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: [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: [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: [PATCH 01/13] apply: mark include/exclude options as NONEG
Jeff King writes:
> The options callback for "git apply --no-include" is not ready to handle
> the "unset" parameter, and as a result will segfault when it adds a NULL
> argument to the include list (likewise for "--no-exclude").
>
> In theory this might be used to clear the list, but since both
> "--include" and "--exclude" add to the same list, it's not immediately
> obvious what the semantics should be. Let's punt on that for now and
> just disallow the broken options.
Thanks. I agree with the conclusion to leave it to later outside
this series to define what --no-(include|exclude) should do.
I suspect something along the lines of
Each element on the single list is marked as either include or
exclude, and "--no-include" would remove the accumulated
"include" entries in the list without touching any "exclude"
elements.
would be sufficiently clear and useful, perhaps.
>
> Signed-off-by: Jeff King
> ---
> apply.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 073d5f0451..d1ca6addeb 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4939,10 +4939,10 @@ int apply_parse_options(int argc, const char **argv,
> struct option builtin_apply_options[] = {
> { OPTION_CALLBACK, 0, "exclude", state, N_("path"),
> N_("don't apply changes matching the given path"),
> - 0, apply_option_parse_exclude },
> + PARSE_OPT_NONEG, apply_option_parse_exclude },
> { OPTION_CALLBACK, 0, "include", state, N_("path"),
> N_("apply changes matching the given path"),
> - 0, apply_option_parse_include },
> + PARSE_OPT_NONEG, apply_option_parse_include },
> { OPTION_CALLBACK, 'p', NULL, state, N_("num"),
> N_("remove leading slashes from traditional diff
> paths"),
> 0, apply_option_parse_p },
Re: [PATCH 02/13] am: handle --no-patch-format option
Jeff King writes:
> Running "git am --no-patch-format" will currently segfault, since it
> tries to parse a NULL argument. Instead, let's have it cancel any
> previous --patch-format option.
Makes perfect sense.
>
> Signed-off-by: Jeff King
> ---
> builtin/am.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 3ee9a9d2a9..dcb880b699 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2165,7 +2165,9 @@ static int parse_opt_patchformat(const struct option
> *opt, const char *arg, int
> {
> int *opt_value = opt->value;
>
> - if (!strcmp(arg, "mbox"))
> + if (unset)
> + *opt_value = PATCH_FORMAT_UNKNOWN;
> + else if (!strcmp(arg, "mbox"))
> *opt_value = PATCH_FORMAT_MBOX;
> else if (!strcmp(arg, "stgit"))
> *opt_value = PATCH_FORMAT_STGIT;
Re: [PATCH] diff: differentiate error handling in parse_color_moved_ws
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.
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, not so excellent (yet) X-<.
Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers)
Anders Waldenborg writes: > + if (opts->separator && first_printed) > + strbuf_addbuf(out, opts->separator); > if (opts->no_key) > - strbuf_addf(out, "%s\n", val.buf); > + strbuf_addf(out, "%s", val.buf); Avoid addf with "%s" alone as a formatter; instead say strbuf_addstr(out, val.buf); cf. contrib/coccinelle/strbuf.cocci
Re: [PATCH v2 2/5] pretty: allow showing specific trailers
Anders Waldenborg writes:
> + else if (skip_prefix(arg, "key=", )) {
> + const char *end = arg + strcspn(arg,
> ",)");
> +
> + if (opts.filter_key)
> + free(opts.filter_key);
Call the free() unconditionally, cf. contrib/coccinelle/free.cocci.
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Eric Sunshine writes: > On Sun, Nov 4, 2018 at 6:26 PM Junio C Hamano wrote: >> 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? >> >> -- >8 -- >> From: Steve Hoelzer >> >> Signed-off-by: Johannes Sixt >> Acked-by: Steve Hoelzer > > It's not clear from this who the author is. Right. The latter should be s-o-b and the order swapped, and probably say "Steve wrote the original version, Johannes extended it" in the text to match. THanks.
Re: [PATCH v2 2/5] pretty: allow showing specific trailers
Eric Sunshine writes: > Does the user have to include the colon when specifying of > 'key='? I can see from peeking at the implementation that the > colon must not be used, but this should be documented. Should the code > tolerate a trailing colon? (Genuine question; it's easy to do and > would be more user-friendly.) > > Does 'key=', do a full or partial match on trailers? And, if > partial, is the match anchored at the start or can it match anywhere > in the trailer key? I see from the implementation that it does a full > match, but this behavior should be documented. > > What happens if 'key=...' is specified multiple times? Are the > multiple keys conjunctive? Disjunctive? Last-wins? I can see from the > implementation that it is last-wins, but this behavior should be > documented. (I wonder how painful it will be for people who want to > match multiple keys. This doesn't have to be answered yet, as the > behavior can always be loosened later to allow multiple-key matching > since the current syntax does not disallow such expansion.) > > Thinking further on the last two points, should be a regular expression? Another thing that needs to be clarified in the document would be case sensitivity. People sometimes spell "Signed-Off-By:" by mistake (or is it by malice?). I do suspect that the parser should just make a list of sought-after keys, not doing "last-one-wins", as that won't be very difficult to do and makes what happens when given multiple keys trivially obvious.
Re: [PATCH v5 00/12] Base SHA-256 implementation
"brian m. carlson" writes: > This series provides a functional SHA-256 implementation and wires it > up, along with some housekeeping patches to make it suitable for > testing. > > Changes from v4: > * Downcase hex constants for consistency. > * Remove needless parentheses in return statement. > * Remove braces for single statement loops. > * Switch to +=. > * Add references to rationale for SHA-256. > * Remove inclusion of "git-compat-util.h" in header. You ended up not doing the last one, judging from the interdiff below. I think it is OK to leave the header in. Thanks, will replace.
Re: Design of multiple hash support
"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
Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers)
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)?
Re: [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function
Anders Waldenborg writes:
> No functional change intended
>
> Signed-off-by: Anders Waldenborg
> ---
> pretty.c | 37 ++---
> 1 file changed, 26 insertions(+), 11 deletions(-)
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".
> diff --git a/pretty.c b/pretty.c
> index f87ba4f18..9fdddce9d 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1074,6 +1074,27 @@ static int match_placeholder_arg(const char *to_parse,
> const char *candidate,
> return 0;
> }
>
> +static size_t format_fundamental(struct strbuf *sb, /* in UTF-8 */
> + const char *placeholder,
> + void *context)
> +{
> + int ch;
> +
> + switch (placeholder[0]) {
> + case 'n': /* newline */
> + strbuf_addch(sb, '\n');
> + return 1;
> + case 'x':
> + /* %x00 == NUL, %x0a == LF, etc. */
> + ch = hex2chr(placeholder + 1);
> + if (ch < 0)
> + return 0;
> + strbuf_addch(sb, ch);
> + return 3;
> + }
> + return 0;
> +}
> +
> static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
> const char *placeholder,
> void *context)
> @@ -1083,9 +1104,13 @@ static size_t format_commit_one(struct strbuf *sb, /*
> in UTF-8 */
> const char *msg = c->message;
> struct commit_list *p;
> const char *arg;
> - int ch;
> + size_t res;
>
> /* these are independent of the commit */
> + res = format_fundamental(sb, placeholder, NULL);
> + if (res)
> + return res;
> +
> switch (placeholder[0]) {
> case 'C':
> if (starts_with(placeholder + 1, "(auto)")) {
> @@ -1104,16 +1129,6 @@ static size_t format_commit_one(struct strbuf *sb, /*
> in UTF-8 */
>*/
> return ret;
> }
> - case 'n': /* newline */
> - strbuf_addch(sb, '\n');
> - return 1;
> - case 'x':
> - /* %x00 == NUL, %x0a == LF, etc. */
> - ch = hex2chr(placeholder + 1);
> - if (ch < 0)
> - return 0;
> - strbuf_addch(sb, ch);
> - return 3;
> case 'w':
> if (placeholder[1] == '(') {
> unsigned long width = 0, indent1 = 0, indent2 = 0;
Re: [PATCH v4 2/5] am: improve author-script error reporting
Eric Sunshine writes:
> Junio labeled the "-2" trick "cute", and while it is optimal in that
> it only traverses the key/value list once, it also increases cognitive
> load since the reader has to spend a good deal more brain cycles
> understanding what is going on than would be the case with simpler
> (and less noisily repetitive) code.
... and update three copies of very similar looking code that have
to stay similar. If this were parsing unbounded and customizable
set of variables, I think the approach you suggest to use a helper
that iterates over the whole array for each key makes sense, but for
now I think what was queued would be OK.
> An alternative would be to make the code trivially easy to understand,
> though a bit more costly, but that expense should be negligible since
> this file should always be tiny, containing very few key/value pairs,
> and it's not timing critical code anyhow. For instance:
>
> static char *find(struct string_list *kv, const char *key)
> {
> const char *val = NULL;
> int i;
> for (i = 0; i < kv.nr; i++) {
> if (!strcmp(kv.items[i].string, key)) {
> if (val) {
> error(_("duplicate %s"), key);
> return NULL;
> }
> val = kv.items[i].util;
> }
> }
> if (!val)
> error(_("missing %s"), key);
> return val;
> }
>
> static int read_author_script(struct am_state *state)
> {
> ...
> char *name, *email, *date;
> ...
> name = find(, "GIT_AUTHOR_NAME");
> email = find(, "GIT_AUTHOR_EMAIL");
> date = find(, "GIT_AUTHOR_DATE");
> if (name && email && date) {
> state->author_name = name;
> state->author_email = email;
> state->author_date = date;
> retval = 0;
> }
> string_list_clear, !!retval);
> ...
Re: [PATCH v2] tree-walk.c: fix overoptimistic inclusion in :(exclude) matching
Nguyễn Thái Ngọc Duy writes: > tree_entry_interesting() is used for matching pathspec on a tree. The > interesting thing about this function is that, because the tree > entries are known to be sorted, this function can return more than > just "yes, matched" and "no, not matched". It can also say "yes, this > entry is matched and so is the remaining entries in the tree". > > This is where I made a mistake when matching exclude pathspec. For > exclude pathspec, we do matching twice, one with positive patterns and > one with negative ones, then a rule table is applied to determine the > final "include or exclude" result. Note that "matched" does not > necessarily mean include. For negative patterns, "matched" means > exclude. > > This particular rule is too eager to include everything. Rule 8 says > that "if all entries are positively matched" and the current entry is > not negatively matched (i.e. not excluded), then all entries are > positively matched and therefore included. But this is not true. If > the _current_ entry is not negatively matched, it does not mean the > next one will not be and we cannot conclude right away that all > remaining entries are positively matched and can be included. > > Rules 8 and 18 are now updated to be less eager. We conclude that the > current entry is positively matched and included. But we say nothing > about remaining entries. tree_entry_interesting() will be called again > for those entries where we will determine entries individually. Thanks. Will queue. > Reported-by: Christophe Bliard > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > v2 fixes the too broad "git add ." in the test > > t/t6132-pathspec-exclude.sh | 17 + > tree-walk.c | 11 --- > 2 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh > index eb829fce97..2462b19ddd 100755 > --- a/t/t6132-pathspec-exclude.sh > +++ b/t/t6132-pathspec-exclude.sh > @@ -194,4 +194,21 @@ test_expect_success 'multiple exclusions' ' > test_cmp expect actual > ' > > +test_expect_success 't_e_i() exclude case #8' ' > + git init case8 && > + ( > + cd case8 && > + echo file >file1 && > + echo file >file2 && > + git add file1 file2 && > + git commit -m twofiles && > + git grep -l file HEAD :^file2 >actual && > + echo HEAD:file1 >expected && > + test_cmp expected actual && > + git grep -l file HEAD :^file1 >actual && > + echo HEAD:file2 >expected && > + test_cmp expected actual > + ) > +' > + > test_done > diff --git a/tree-walk.c b/tree-walk.c > index 77b37f36fa..79bafbd1a2 100644 > --- a/tree-walk.c > +++ b/tree-walk.c > @@ -1107,7 +1107,7 @@ enum interesting tree_entry_interesting(const struct > name_entry *entry, >* 5 | file |1 |1 | 0 >* 6 | file |1 |2 | 0 >* 7 | file |2 | -1 | 2 > - * 8 | file |2 |0 | 2 > + * 8 | file |2 |0 | 1 >* 9 | file |2 |1 | 0 >* 10 | file |2 |2 | -1 >* -+---+--+--+--- > @@ -1118,7 +1118,7 @@ enum interesting tree_entry_interesting(const struct > name_entry *entry, >* 15 | dir |1 |1 | 1 (*) >* 16 | dir |1 |2 | 0 >* 17 | dir |2 | -1 | 2 > - * 18 | dir |2 |0 | 2 > + * 18 | dir |2 |0 | 1 >* 19 | dir |2 |1 | 1 (*) >* 20 | dir |2 |2 | -1 >* > @@ -1134,7 +1134,12 @@ enum interesting tree_entry_interesting(const struct > name_entry *entry, > > negative = do_match(entry, base, base_offset, ps, 1); > > - /* #3, #4, #7, #8, #13, #14, #17, #18 */ > + /* #8, #18 */ > + if (positive == all_entries_interesting && > + negative == entry_not_interesting) > + return entry_interesting; > + > + /* #3, #4, #7, #13, #14, #17 */ > if (negative <= entry_not_interesting) > return positive;
Re: [PATCH] sequencer.c: remove a stray semicolon
Nguyễn Thái Ngọc Duy writes: > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > On top of ag/rebase-i-in-c Thanks.
Re: [PATCH] git-worktree.txt: correct linkgit command name
Nguyễn Thái Ngọc Duy writes: > Noticed-by: SZEDER Gábor > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > On top of nd/per-worktree-ref-iteration Thanks. > > Documentation/git-worktree.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > index 9117e4fb50..69d55f1350 100644 > --- a/Documentation/git-worktree.txt > +++ b/Documentation/git-worktree.txt > @@ -230,7 +230,7 @@ GIT_COMMON_DIR/worktrees/foo/HEAD and > GIT_COMMON_DIR/worktrees/bar/refs/bisect/bad. > > To access refs, it's best not to look inside GIT_DIR directly. Instead > -use commands such as linkgit:git-revparse[1] or linkgit:git-update-ref[1] > +use commands such as linkgit:git-rev-parse[1] or linkgit:git-update-ref[1] > which will handle refs correctly. > > DETAILS
Re: [PATCH v2] build: link with curl-defined linker flags
James Knight writes: > Changes v1 -> v2: > - Improved support for detecting curl linker flags when not using a > configure-based build (mentioned by Junio C Hamano). > - Adding a description on how to explicitly use the CURL_LDFLAGS > define when not using configure (suggested by Junio C Hamano). > > The original patch made a (bad) assumption that builds would always > invoke ./configure before attempting to build Git. To support a > make-invoked build, CURL_LDFLAGS can also be populated using the defined > curl-config utility. This change also comes with the assumption that > since both ./configure and Makefile are using curl-config to determine > aspects of the build, it should be also fine to use the same utility to > provide the linker flags to compile against (please indicate so if this > is another bad assumption). With this change, the explicit configuration > of CURL_LDFLAGS inside config.mak.uname for Minix and NONSTOP_KERNEL > have been dropped. Will replace; thanks. > Makefile | 30 +++--- > config.mak.uname | 3 --- > configure.ac | 17 +++-- > 3 files changed, 22 insertions(+), 28 deletions(-) > > diff --git a/Makefile b/Makefile > index > b08d5ea258c69a78745dfa73fe698c11d021858a..36da17dc1f9b1a70c9142604afe989f1eb8ee87f > 100644 > --- a/Makefile > +++ b/Makefile > @@ -59,6 +59,13 @@ all:: > # Define CURL_CONFIG to curl's configuration program that prints information > # about the library (e.g., its version number). The default is > 'curl-config'. > # > +# Define CURL_LDFLAGS to specify flags that you need to link when using > libcurl, > +# if you do not want to rely on the libraries provided by CURL_CONFIG. The > +# default value is a result of `curl-config --libs`. An example value for > +# CURL_LDFLAGS is as follows: > +# > +# CURL_LDFLAGS=-lcurl > +# > # Define NO_EXPAT if you do not have expat installed. git-http-push is > # not built, and you cannot push using http:// and https:// transports > (dumb). > # > @@ -183,10 +190,6 @@ all:: > # > # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto > (Darwin). > # > -# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix). > -# > -# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix). > -# > # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin). > # > # Define NEEDS_LIBINTL_BEFORE_LIBICONV if you need libintl before libiconv. > @@ -1305,20 +1308,17 @@ else > ifdef CURLDIR > # Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case. > BASIC_CFLAGS += -I$(CURLDIR)/include > - CURL_LIBCURL = -L$(CURLDIR)/$(lib) > $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl > + CURL_LIBCURL = -L$(CURLDIR)/$(lib) > $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) > else > - CURL_LIBCURL = -lcurl > - endif > - ifdef NEEDS_SSL_WITH_CURL > - CURL_LIBCURL += -lssl > - ifdef NEEDS_CRYPTO_WITH_SSL > - CURL_LIBCURL += -lcrypto > - endif > - endif > - ifdef NEEDS_IDN_WITH_CURL > - CURL_LIBCURL += -lidn > + CURL_LIBCURL = > endif > > +ifdef CURL_LDFLAGS > + CURL_LIBCURL += $(CURL_LDFLAGS) > +else > + CURL_LIBCURL += $(shell $(CURL_CONFIG) --libs) > +endif > + > REMOTE_CURL_PRIMARY = git-remote-http$X > REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X > git-remote-ftps$X > REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES) > diff --git a/config.mak.uname b/config.mak.uname > index > 8acdeb71fdab3b3e8e3c536110eb6de13f14e8ff..19e6633040b1db4a148d7b33c4e9d374fe7f87ba > 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -431,8 +431,6 @@ ifeq ($(uname_S),Minix) > NO_NSEC = YesPlease > NEEDS_LIBGEN = > NEEDS_CRYPTO_WITH_SSL = YesPlease > - NEEDS_IDN_WITH_CURL = YesPlease > - NEEDS_SSL_WITH_CURL = YesPlease > NEEDS_RESOLV = > NO_HSTRERROR = YesPlease > NO_MMAP = YesPlease > @@ -458,7 +456,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL) > # Missdetected, hence commented out, see below. > #NO_CURL = YesPlease > # Added manually, see above. > - NEEDS_SSL_WITH_CURL = YesPlease > HAVE_LIBCHARSET_H = YesPlease > HAVE_STRINGS_H = YesPlease > NEEDS_LIBICONV = YesPlease > diff --git a/configure.ac b/configure.ac > index > e11b7976ab1c93d8ccec2e499d0093db42551059..44e8c036b6ec417e95ca4e5c2861785900d8634c > 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -600,17 +600,1
Re: "git checkout" safety feature
Matthias Urlichs writes: > A recent discussion on LWN https://lwn.net/Articles/770642/ noted that > "git checkout " does not warn if one if the files has > been modified locally, nor is there an option to do so. > > IMHO that should be fixed, preferably by somebody who knows git's > internals well enough to do so in half an hour ;-) "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?
Re: [PATCH 2/2] t/t7510-signed-commit.sh: add signing subkey to Eris Discordia key
Michał Górny writes: >> It's my understanding that GnuPG will use the most recent subkey >> suitable for a particular purpose, and I think the test relies on that >> behavior. However, I'm not sure that's documented. Do we want to rely >> on that behavior or be more explicit? (This is a question, not an >> opinion.) > > To be honest, I don't recall which suitable subkey is used. However, it > definitely will prefer a subkey with signing capabilities over > the primary key if one is present, and this is well-known and expected > behavior. > > In fact, if you have a key with two signing subkeys A and B and it > considers A better, then even if you explicitly pass keyid of B, it will > use A. To force another subkey you have to append '!' to keyid. > > Therefore, I think this is a behavior we can rely on. I didn't check how the signing key configuration is done in the test sript (which is outside the patch context), but do you mean that we create these signed objects by specifying which key to use with a keyid with "!" appended? If so I agree that would make sense, because we would then know which subkey should be used for signing and checking with %GF/%GP would be a good way to do so. Thanks.
Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines
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.
> 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.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> I think standardizing how we record commit ids in the commit message
> is a good idea. Though to be honest this started because of my irk of
> an English string "cherry picked from..." that cannot be translated.
> It might as well be a computer language that happens to look like
> English.
>
> Documentation/git-cherry-pick.txt | 5 ++---
> sequencer.c | 20 ++--
> t/t3510-cherry-pick-sequence.sh | 12 ++--
> t/t3511-cherry-pick-x.sh | 26 +-
> 4 files changed, 27 insertions(+), 36 deletions(-)
>
> diff --git a/Documentation/git-cherry-pick.txt
> b/Documentation/git-cherry-pick.txt
> index d35d771fc8..8ffbaed1a0 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -58,9 +58,8 @@ OPTIONS
> message prior to committing.
>
> -x::
> - When recording the commit, append a line that says
> - "(cherry picked from commit ...)" to the original commit
> - message in order to indicate which commit this change was
> + When recording the commit, append "Cherry-Pick:" trailer line
> + recording the commit name which commit this change was
> cherry-picked from. This is done only for cherry
> picks without conflicts. Do not use this option if
> you are cherry-picking from your private branch because
> diff --git a/sequencer.c b/sequencer.c
> index 9e1ab3a2a7..f7318f2862 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -36,7 +36,6 @@
> #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>
> const char sign_off_header[] = "Signed-off-by: ";
> -static const char cherry_picked_prefix[] = "(cherry picked from commit ";
>
> GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
>
> @@ -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));
> - }
> - strbuf_addstr(, ".\n");
> + strbuf_addf(, "Revert \"%s\"\n\n", msg.subject);
> +
> + strbuf_addf(, "Revert: %s\n",
> + oid_to_hex(>object.oid));
> } else {
> const char *p;
>
> @@ -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));
> }
> if (!is_fixup(command))
> author = get_author(msg.message);
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index c84eeefdc9..89b6e7fc1e 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -390,10 +390,10 @@ test_expect_success '--continue respects opts' '
> git cat-file commit HEAD~1 >picked_msg &&
> git cat-file commit HEAD~2 >unrelatedpick_msg &&
> git cat-file commit HEAD~3 >initial_msg &&
> - ! grep "cherry picked from" initial_msg &&
> - grep "cherry picked from" unrelatedpick_msg &&
> - grep "cherry picked from" picked_msg &&
> - grep "cherry picked from" anotherpick_msg
> + ! grep "Cherry-Pick:" initial_msg &&
> +
Re: [RFC PATCH] checkout: add synonym of -b
Mikkel Kjeldsen writes: > Add --new-branch as a long-form synonym of -b. I occasionally encounter > some confusion in new users having interpreted "checkout -b" to mean > "checkout branch", or internalized it as "the way to create a new > branch" rather than merely a convenience for "branch && checkout". I > think an explicit long-form can help alleviate that. > > Signed-off-by: Mikkel Kjeldsen > --- > > Notes: > This makes the synopsis and description lines look a little clumsy (and > I think incorrect...?) so if this proposal is accepted perhaps those > parts are better left out. It is meant more for training and > documentation than regular usage, anyway. Sounds like even you (who wrote this patch) expect the long form option to be impractical for regular usage and everybody would end up using the -b form? I am borderline "Meh" on this change, slightly on the negative side, primarily because we'd need to worry about what to do with "-B" if we did this to "-b", and I do not think it is worth even spending brain cycles to worry about it (e.g. should it be --force-new-branch? should --new-branch used together with --force a better-looking alternative? if we were to add --force, how should it interact with other existing options the command support? etc.) But let's hear what others think. Thanks.
Re: [PATCH v3] commit: add a commit.allowEmpty config variable
Ævar Arnfjörð Bjarmason writes:
> I.e. you seemingly have no interest in using "git commit" to produce
> empty commits, but are just trying to cherry-pick something and it's
> failing because it (presumably, or am I missing something) cherry picks
> an existing commit content ends up not changing anything.
>
> I.e. you'd like to make the logic 37f7a85793 ("Teach commit about
> CHERRY_PICK_HEAD", 2011-02-19) added a message for the default.
>
> So let's talk about that use case, and for those of us less familiar
> with this explain why it is that this needs to still be optional at
> all. I.e. aren't we just exposing an implementation detail here where
> cherry-pick uses the commit machinery? Should we maybe just always pass
> --allow-empty on cherry-pick, if not why not?
>
> I can think of some reasons, but the above is a hint that both this
> patch + the current documentation which talks about "foreign SCM
> scripts" have drifted very far from what this is actually being used
> for, so let's update that.
The command line "--allowAnything" in general is meant to be an
escape hatch for unusual situations, and if a workflow requires
constant use of that escape hatch, there is something wrong either
in the workflow or in the tool used in the workflow, and it is what
we should first see if we can fix, I would think, before making it
easy to constantly use the escape hatch.
I didn't look at the external reference you looked at but it sounds
like your review comment is taking the topic in the right direction.
Thanks for digging for the backstory.
Re: [RFC v1] Add virtual file system settings and hook proc
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?
> 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 ;-)
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Johannes Sixt writes:
> Am 03.11.18 um 09:14 schrieb Carlo Arenas:
>> On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt wrote:
>>>
>>> + timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout -
>>> elapsed);
>>
>> nitpick: cast to DWORD instead of int
>
> No; timeout is of type int; after an explicit type cast we don't want
> to have another implicit conversion.
>
> -- Hannes
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?
-- >8 --
From: Steve Hoelzer
Date: Wed, 31 Oct 2018 14:11:36 -0700
Subject: [PATCH] poll: use GetTickCount64() to avoid wrap-around issues
The value of timeout starts as an int value, and for this reason it
cannot overflow unsigned long long aka ULONGLONG. The unsigned version
of this initial value is available in orig_timeout. The difference
(orig_timeout - elapsed) cannot wrap around because it is protected by
a conditional (as can be seen in the patch text). Hence, the ULONGLONG
difference can only have values that are smaller than the initial
timeout value and truncation to int cannot overflow.
Signed-off-by: Johannes Sixt
Acked-by: Steve Hoelzer
Signed-off-by: Junio C Hamano
---
compat/poll/poll.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/compat/poll/poll.c b/compat/poll/poll.c
index ad5dcde439..4459408c7d 100644
--- a/compat/poll/poll.c
+++ b/compat/poll/poll.c
@@ -18,6 +18,9 @@
You should have received a copy of the GNU General Public License along
with this program; if not, see <http://www.gnu.org/licenses/>. */
+/* To bump the minimum Windows version to Windows Vista */
+#include "git-compat-util.h"
+
/* Tell gcc not to warn about the (nfd < 0) tests, below. */
#if (__GNUC__ == 4 && 3 <= __GNUC_MINOR__) || 4 < __GNUC__
# pragma GCC diagnostic ignored "-Wtype-limits"
@@ -449,7 +452,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
static HANDLE hEvent;
WSANETWORKEVENTS ev;
HANDLE h, handle_array[FD_SETSIZE + 2];
- DWORD ret, wait_timeout, nhandles, start = 0, elapsed, orig_timeout = 0;
+ DWORD ret, wait_timeout, nhandles, orig_timeout = 0;
+ ULONGLONG start = 0;
fd_set rfds, wfds, xfds;
BOOL poll_again;
MSG msg;
@@ -465,7 +469,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
if (timeout != INFTIM)
{
orig_timeout = timeout;
- start = GetTickCount();
+ start = GetTickCount64();
}
if (!hEvent)
@@ -614,8 +618,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
if (!rc && orig_timeout && timeout != INFTIM)
{
- elapsed = GetTickCount() - start;
- timeout = elapsed >= orig_timeout ? 0 : orig_timeout - elapsed;
+ ULONGLONG elapsed = GetTickCount64() - start;
+ timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);
}
if (!rc && timeout)
--
2.19.1-816-gcd69ec8cde
Re: Git Test Coverage Report (Friday, Nov 2)
Michał Górny writes: > I've just did a little research and came to the following results: Wonderful. > 1. modifying the 'C. O. Mitter' key would require changes to 4 tests, > > 2. modifying the 'Eris Discordia' key would require changes to 2 tests >(both in 7510). > > Do you think 2. would be an acceptable option? Yeah, that sounds like the best way to go. Thanks for digging.
Re: [PATCH v2] completion: use builtin completion for format-patch
Duy Nguyen writes: >> Would it make sense to make send-email's completion helper print these >> out directly? That way, if someone were to modify send-email in the >> future, they'd only have to look through one file instead of both >> send-email and the completions script. > > I did think about that and decided not to do it (in fact the first > revision of this patch did exactly that). > > If we have to maintain this list manually, we might as well leave to > the place that matters: the completion script. I don't think the > person who updates send-email.perl would be always interested in > completion support. And the one that is interested usually only looks > at the completion script. Putting this list in send-email.perl just > makes it harder to find. I do not necessarily disagree with the conclusion, but I am not sure if I agree with the last paragraph. If the definition used to list completable options was in the send-email command, it is more likely that a patch to send-email.perl that would add/modify an option without making a matching change to the definition in the same file gets noticed, whether the developer who is doing the feature is or is not interested in maintaining the completion script working, no? Similarly, if one notices that an option the command supports that ought to get completed does not get completed, and gets motivated enough to try finding where in the completion script other existing options that do get completed are handled, wouldn't that lead one to the right solution, i.e. discovery of the definition in the send-email script? Quite honestly, I would expect that our developers and user base are much more competent than one who - wants to add completion of the option Y to the command A, which has known-to-be-working completion of the option X, and yet - fails to imagine that it could be a possible good first step to figure out how the option X is completed, so that a new support for the option Y might be able to emulate it. Now, once we start going in the direction of having both the implementation of options *and* the definition of the list of completable options in send-email.perl script, I would agree with your initial assessment in a message much earlier in the thread. It would be very tempting to use the data we feed Getopt::Long as the source of the list of completable options to reduce the longer-term maintenance load, which would mean it will involve more work. And in order to avoid having to invest more work upfront (which I do not think is necessarily a bad thing), having the definition in the completion script might be easier to manage---it is closer to the status quo, especially the state before you taught parse-options API to give the list of completable options. Thanks.
Re: Git Test Coverage Report (Friday, Nov 2)
Michał Górny writes: > As for how involved... we'd just have to use a key that has split > signing subkey. Would it be fine to add the subkey to the existing key? > It would imply updating keyids/fingerprints everywhere. Yes, that "everywhere" is exactly what I meant by "how involved", and your suggestion answers "very much involved". If we can easily add _another_ key with a subkey that is not the primary one we use for other tests, without touching the existing key and the existing tests that use it (including the one I touched below--- we'd want to see a sig with a key that is not split is shown with the same %GF and %GP), while adding a handful of new tests that create signed objects under the new & split key and view them with %GF and %GP, then the end result would be that we managed to add a new test case where %GF/%GP are different without making very much involved changes. I guess that was what I was getting at. Thanks. > >> >> t/t7510-signed-commit.sh | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh >> index 19ccae2869..9ecafedcc4 100755 >> --- a/t/t7510-signed-commit.sh >> +++ b/t/t7510-signed-commit.sh >> @@ -176,8 +176,9 @@ test_expect_success GPG 'show good signature with custom >> format' ' >> 13B6F51ECDDE430D >> C O Mitter >> 73D758744BE721698EC54E8713B6F51ECDDE430D >> +73D758744BE721698EC54E8713B6F51ECDDE430D >> EOF >> -git log -1 --format="%G?%n%GK%n%GS%n%GF" sixth-signed >actual && >> +git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual && >> test_cmp expect actual >> ' >>
Re: Git Test Coverage Report (Friday, Nov 2)
Derrick Stolee writes:
> Uncovered code in 'next' not in 'master'
>
>
> pretty.c
> 4de9394dcb 1264) if (c->signature_check.primary_key_fingerprint)
> 4de9394dcb 1265) strbuf_addstr(sb,
> c->signature_check.primary_key_fingerprint);
> 4de9394dcb 1266) break;
Perhaps a patch along this line can be appended to the
mg/gpg-fingerprint topic that ends at 4de9394d ("gpg-interface.c:
obtain primary key fingerprint as well", 2018-10-22) to cover this
entry in the report.
I do not know how involved it would be to set up a new test case
that demonstrates a case where %GF and %GP are different, but if it
is very involved perhaps it is not worth adding such a case.
t/t7510-signed-commit.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 19ccae2869..9ecafedcc4 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -176,8 +176,9 @@ test_expect_success GPG 'show good signature with custom
format' '
13B6F51ECDDE430D
C O Mitter
73D758744BE721698EC54E8713B6F51ECDDE430D
+ 73D758744BE721698EC54E8713B6F51ECDDE430D
EOF
- git log -1 --format="%G?%n%GK%n%GS%n%GF" sixth-signed >actual &&
+ git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
test_cmp expect actual
'
Re: [PATCH] diff: differentiate error handling in parse_color_moved_ws
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?
Thanks.
Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
Æ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'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. 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". > Æ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 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Junio C Hamano writes: > Johannes Sixt writes: > >>> Yep, correct on all counts. I'm in favor of changing the commit message to >>> only say that this patch removes Warning C28159. >> >> How about this fixup instead? > > Isn't that already in 'next'? I didn't check, though. Well, it turnsout that I already prepared one but not pushed it out yet. I'll eject this topic and rebuild the integration branches, and wait until Dscho says something, to avoid having to redo the integration cycle again. Thanks.
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
Johannes Sixt writes: >> Yep, correct on all counts. I'm in favor of changing the commit message to >> only say that this patch removes Warning C28159. > > How about this fixup instead? Isn't that already in 'next'? I didn't check, though.
Re: [PATCH v1] add: speed up cmd_add() by utilizing read_cache_preload()
Duy Nguyen writes: > On Fri, Nov 2, 2018 at 2:32 PM Ben Peart wrote: >> >> From: Ben Peart >> >> During an "add", a call is made to run_diff_files() which calls >> check_remove() for each index-entry. The preload_index() code distributes >> some of the costs across multiple threads. > > Instead of doing this site by site. How about we make read_cache() > always do multithread preload? I suspect that it would be a huge performance killer. Many codepaths do not even want to know if the working tree files have been modified, even though they need to know what's in the index. Think "git commit-tree", "git diff --cached", etc.
Re: [PATCH 3/9] diff: avoid generating unused hunk header lines
Jeff King writes: >> to have *some* names there, for the sake of a >> simply described coding style without many exceptions >> (especially those exceptions that rely on judgement). > > Fair enough. For the record, there aren't "many" exceptions to the rule that was suggested earlier: if you refer to parameters by name in the comment to explain the interface, name them and use these names [*1*]. > Squashable patch is below; it goes on 34c829621e (diff: avoid generating > unused hunk header lines, 2018-11-02). > > Junio, let me know if you'd prefer a re-send of the series, but it > doesn't look necessary otherwise (so far). Giving them proper names would serve as a readily usable sample for those who write new hunk-line callbacks that do not ignore them, so what you wrote is good. Let me squash it in. Thanks. If this were to add bunch of "unused$n" names to the parameters to this no-op function, only to "have *some* names there", I would have said that it is not even worth the time to discuss it, though. > diff --git a/xdiff-interface.h b/xdiff-interface.h > index 7b0ccbdd1d..8af245eed9 100644 > --- a/xdiff-interface.h > +++ b/xdiff-interface.h > @@ -39,7 +39,9 @@ extern int git_xmerge_style; > * Can be used as a no-op hunk_fn for xdi_diff_outf(), since a NULL > * one just sends the hunk line to the line_fn callback). > */ > -void discard_hunk_line(void *, long, long, long, long, const char *, long); > +void discard_hunk_line(void *priv, > +long ob, long on, long nb, long nn, > +const char *func, long funclen); > > /* > * Compare the strings l1 with l2 which are of size s1 and s2 respectively. > > -Peff [Footnote] *1* You may say that the "if you refer to parameters by name" part relies on judgment, but I think it is a good thing---it makes poor judgment stand out. When describing a function that takes two parameters of the same type, for example, you could explain the interface as "take two ints and return true if first int is smaller than the second int" in order to leave the parameters unnamed, but if you gave such a description, it is so obvious that you are _avoiding_ names. Not using names when you do not have to is good, but nobody with half an intelligence would think it is good to give a bad description just to avoid using names.
Re: [PATCH v1] add: speed up cmd_add() by utilizing read_cache_preload()
Ben Peart writes:
> From: Ben Peart
>
> During an "add", a call is made to run_diff_files() which calls
> check_remove() for each index-entry. The preload_index() code
> distributes some of the costs across multiple threads.
Nice. I peeked around and noticed that we already do this in
builtin_diff_index() before running run_diff_index() when !cached,
and builtin_diff_files(), of course.
> Because the files checked are restricted to pathspec, adding individual
> files makes no measurable impact but on a Windows repo with ~200K files,
> 'git add .' drops from 6.3 seconds to 3.3 seconds for a 47% savings.
;-)
> diff --git a/builtin/add.c b/builtin/add.c
> index ad49806ebf..f65c172299 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -445,11 +445,6 @@ int cmd_add(int argc, const char **argv, const char
> *prefix)
> return 0;
> }
>
> - if (read_cache() < 0)
> - die(_("index file corrupt"));
> -
> - die_in_unpopulated_submodule(_index, prefix);
> -
> /*
>* Check the "pathspec '%s' did not match any files" block
>* below before enabling new magic.
It is not explained why this is not a mere s/read_cache/&_preload/
in the log message. I can see it is because you wanted to make the
pathspec available to preload to further cut down the preloaded
paths, and I do not think it has any unintended (negatie) side
effect to parse the pathspec before populating the in-core index,
but that would have been a good thing to mention in the proposed log
message.
> @@ -459,6 +454,10 @@ int cmd_add(int argc, const char **argv, const char
> *prefix)
> PATHSPEC_SYMLINK_LEADING_PATH,
> prefix, argv);
>
> + if (read_cache_preload() < 0)
> + die(_("index file corrupt"));
> +
> + die_in_unpopulated_submodule(_index, prefix);
> die_path_inside_submodule(_index, );
>
> if (add_new_files) {
>
> base-commit: 4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2
Re: [PATCH] build: link with curl-defined linker flags
James Knight writes: > Makefile | 21 +++-- > config.mak.uname | 5 ++--- > configure.ac | 17 +++-- > 3 files changed, 16 insertions(+), 27 deletions(-) > > diff --git a/Makefile b/Makefile > index b08d5ea25..c3be87b0e 100644 > --- a/Makefile > +++ b/Makefile > @@ -183,10 +183,6 @@ all:: > # > # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto > (Darwin). > # > -# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix). > -# > -# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix). > -# Hmm, because the use of autoconf -> ./configure in our build procedure is optional, wouldn't this change give regression to those of us who use these Makefile variables to configure their build, instead of relying on autoconf? > diff --git a/config.mak.uname b/config.mak.uname > index 8acdeb71f..923b8fa09 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -431,8 +431,7 @@ ifeq ($(uname_S),Minix) > NO_NSEC = YesPlease > NEEDS_LIBGEN = > NEEDS_CRYPTO_WITH_SSL = YesPlease > - NEEDS_IDN_WITH_CURL = YesPlease > - NEEDS_SSL_WITH_CURL = YesPlease > + CURL_LDFLAGS = -lssl -lcrypto -lidn OK, as long as we describe how to update their config.mak to adjust to the new world order, the "regression" I noticed earlier is not too bad, and the way the new CURL_LDFLAGS variable drives the build is much more direct and transparent than via the old way to specify NEEDS_*_WITH_CURL to affect the build indirectly. I think such "describe how to configure in the new world order" should go to where NEEDS_*_WITH_CURL used to be, e.g. "Define CURL_LDFLAGS to specify flags that you need to link using -lcurl; see config.mak.uname and look for the variable for examples" or something like that, perhaps. Thanks.
Re: [PATCH] it's called read_object_file() these days
Jeff King writes: > There's another mention in Documentation/technical/api-object-access.txt. Yes, and we are on the same page on that one. > > But since the entire API is undocumented, I'm not sure it matters much. > That file has been a placeholder since 2007. Maybe we should just delete > it; its existence does not seem to be guilting anyone into documenting, > and these days we'd prefer to do it in-header anyway. > > -Peff
Re: [PATCH 0/9] saner xdiff hunk callbacks
Jeff King writes: > ... it seems rather silly that we have xdiff generate a string > just so that we can parse it from within the same process. So instead I > improved the xdiff interface to pass the actual integers out, made use > of it as appropriate, and then in the final patch we can just drop the > buggy function entirely. Acked-by: me. Thanks. This is exactly what I wanted to do for a very long time, ever since I did the original xdiff-interface.c
Re: Understanding pack format
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. Personally I find the latter a bit too dense for those who are new to the codebase, and the former would probably be easier to grok. Start from sha1-file.c::read_object(), which will eventually lead you to oid_object_info_extended() that essentially boils down to - a call to find_pack_entry() with the object name, and then - a call to packed_object_info() with the pack entry found earlier. Following packfile.c::packed_object_info() will lead you to cache_or_unpack_entry(); the unpack_entry() function is where all the action to read from the packstream for one object's worth of data and to reconstruct the object out of its deltified representation takes place.
[PATCH] it's called read_object_file() these days
Remnant of the old name of the function still remains in comments.
Update them all.
Signed-off-by: Junio C Hamano
---
apply.c | 2 +-
builtin/gc.c | 2 +-
fast-import.c | 4 ++--
notes.c | 2 +-
object.h | 2 +-
sha1-file.c | 2 +-
6 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/apply.c b/apply.c
index 073d5f0451..ef1a1b2c4e 100644
--- a/apply.c
+++ b/apply.c
@@ -3254,7 +3254,7 @@ static int read_blob_object(struct strbuf *buf, const
struct object_id *oid, uns
result = read_object_file(oid, , );
if (!result)
return -1;
- /* XXX read_sha1_file NUL-terminates */
+ /* XXX read_object_file NUL-terminates */
strbuf_attach(buf, result, sz, sz + 1);
}
return 0;
diff --git a/builtin/gc.c b/builtin/gc.c
index 871a56f1c5..a682a0f44e 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -285,7 +285,7 @@ static uint64_t estimate_repack_memory(struct packed_git
*pack)
/* revindex is used also */
heap += sizeof(struct revindex_entry) * nr_objects;
/*
-* read_sha1_file() (either at delta calculation phase, or
+* read_object_file() (either at delta calculation phase, or
* writing phase) also fills up the delta base cache
*/
heap += delta_base_cache_limit;
diff --git a/fast-import.c b/fast-import.c
index 95600c78e0..f73b2ae0a6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1298,13 +1298,13 @@ static void stream_blob(uintmax_t len, struct object_id
*oidout, uintmax_t mark)
* oe must not be NULL. Such an oe usually comes from giving
* an unknown SHA-1 to find_object() or an undefined mark to
* find_mark(). Callers must test for this condition and use
- * the standard read_sha1_file() when it happens.
+ * the standard read_object_file() when it happens.
*
* oe->pack_id must not be MAX_PACK_ID. Such an oe is usually from
* find_mark(), where the mark was reloaded from an existing marks
* file and is referencing an object that this fast-import process
* instance did not write out to a packfile. Callers must test for
- * this condition and use read_sha1_file() instead.
+ * this condition and use read_object_file() instead.
*/
static void *gfi_unpack_entry(
struct object_entry *oe,
diff --git a/notes.c b/notes.c
index 25cdce28b7..6a430931a3 100644
--- a/notes.c
+++ b/notes.c
@@ -858,7 +858,7 @@ static int string_list_add_note_lines(struct string_list
*list,
if (is_null_oid(oid))
return 0;
- /* read_sha1_file NUL-terminates */
+ /* read_object_file NUL-terminates */
data = read_object_file(oid, , );
if (t != OBJ_BLOB || !data || !len) {
free(data);
diff --git a/object.h b/object.h
index 0feb90ae61..4cabc9f278 100644
--- a/object.h
+++ b/object.h
@@ -136,7 +136,7 @@ struct object *parse_object(struct repository *r, const
struct object_id *oid);
*/
struct object *parse_object_or_die(const struct object_id *oid, const char
*name);
-/* Given the result of read_sha1_file(), returns the object after
+/* Given the result of read_object_file(), returns the object after
* parsing it. eaten_p indicates if the object has a borrowed copy
* of buffer and the caller should not free() it.
*/
diff --git a/sha1-file.c b/sha1-file.c
index dd0b6aa873..31c2b926fe 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -124,7 +124,7 @@ const char *empty_blob_oid_hex(void)
/*
* This is meant to hold a *small* number of objects that you would
- * want read_sha1_file() to be able to return, but yet you do not want
+ * want read_object_file() to be able to return, but yet you do not want
* to write them into the object store (e.g. a browse-only
* application).
*/
Re: [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function
Junio C Hamano writes: > As we currently have no idea when builtin/stash.c becomes ready for > 'next', how about doing something like this instead, in order to > help end-users without waiting in the meantime? The fix can be > picked up and ported when the C rewrite is updated, of course. I think a better approach to fix the current C version, assuming that a reroll won't change the structure of the code too much and keeps using commit_tree() to synthesize the stash entries, would be to teach commit_tree() -> commit_tree_extended() codepath to take commiter identity just like it takes author identity. Then inside builtin/stash.c, we can choose what committer and author identity to pass without having commit_tree_extended() ask for identity with the STRICT option. Right now, commit_tree() does not have a good way, other than somehow lying to git_author/committer_info(), to record a commit created under an arbitrary identity, which would be fixed with such an approach and will help callers with similar needs in the future.
What's cooking in git.git (Nov 2018, #02; Fri, 2)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. The eighth batch is all about "rebase and rebase-i in C" ;-) The disruptive "let's split up the Documentation/config.txt file" topic will enter 'next' soon, and I expect I'd be slow while it skips over various topics in flight that changes the source it touches, as they would produce conflicts with different shape and won't be helped by rerere. Let's merge it down quickly. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * ag/rebase-i-in-c (2018-10-09) 20 commits (merged to 'next' on 2018-10-19 at 1b24712d46) + rebase -i: move rebase--helper modes to rebase--interactive + rebase -i: remove git-rebase--interactive.sh + rebase--interactive2: rewrite the submodes of interactive rebase in C + rebase -i: implement the main part of interactive rebase as a builtin + rebase -i: rewrite init_basic_state() in C + rebase -i: rewrite write_basic_state() in C + rebase -i: rewrite the rest of init_revisions_and_shortrevisions() in C + rebase -i: implement the logic to initialize $revisions in C + rebase -i: remove unused modes and functions + rebase -i: rewrite complete_action() in C + t3404: todo list with commented-out commands only aborts + sequencer: change the way skip_unnecessary_picks() returns its result + sequencer: refactor append_todo_help() to write its message to a buffer + rebase -i: rewrite checkout_onto() in C + rebase -i: rewrite setup_reflog_action() in C + sequencer: add a new function to silence a command, except if it fails + rebase -i: rewrite the edit-todo functionality in C + editor: add a function to launch the sequence editor + rebase -i: rewrite append_todo_help() in C + sequencer: make three functions and an enum from sequencer.c public (this branch is used by ag/sequencer-reduce-rewriting-todo, cb/printf-empty-format, js/rebase-autostash-fix, js/rebase-i-break, js/rebase-i-shortopt, js/rebase-in-c-5.5-work-with-rebase-i-in-c and pk/rebase-in-c-6-final.) Rewrite of the remaining "rebase -i" machinery in C. * cb/printf-empty-format (2018-10-27) 1 commit (merged to 'next' on 2018-11-01 at 9fcb05f22c) + sequencer: cleanup for gcc warning in non developer mode (this branch uses ag/rebase-i-in-c; is tangled with ag/sequencer-reduce-rewriting-todo, js/rebase-autostash-fix, js/rebase-i-break, js/rebase-i-shortopt, js/rebase-in-c-5.5-work-with-rebase-i-in-c and pk/rebase-in-c-6-final.) Build fix for a topic in flight. * jc/rebase-in-c-5-test-typofix (2018-10-11) 1 commit (merged to 'next' on 2018-10-19 at 08c9d86ffd) + rebase: fix typoes in error messages (this branch uses pk/rebase-in-c, pk/rebase-in-c-2-basic, pk/rebase-in-c-3-acts, pk/rebase-in-c-4-opts and pk/rebase-in-c-5-test; is tangled with js/rebase-autostash-fix, js/rebase-in-c-5.5-work-with-rebase-i-in-c and pk/rebase-in-c-6-final.) Typofix. * js/rebase-autostash-fix (2018-10-24) 5 commits (merged to 'next' on 2018-10-29 at 680e648001) + rebase --autostash: fix issue with dirty submodules + rebase --autostash: demonstrate a problem with dirty submodules + rebase (autostash): use an explicit OID to apply the stash + rebase (autostash): store the full OID in /autostash + rebase (autostash): avoid duplicate call to state_dir_path() (this branch uses ag/rebase-i-in-c, js/rebase-in-c-5.5-work-with-rebase-i-in-c, pk/rebase-in-c, pk/rebase-in-c-2-basic, pk/rebase-in-c-3-acts, pk/rebase-in-c-4-opts, pk/rebase-in-c-5-test and pk/rebase-in-c-6-final; is tangled with ag/sequencer-reduce-rewriting-todo, cb/printf-empty-format, jc/rebase-in-c-5-test-typofix, js/rebase-i-break and js/rebase-i-shortopt.) "git rebase" that has recently been rewritten in C had a few issues in its "--autstash" feature, which have been corrected. * js/rebase-i-break (2018-10-12) 2 commits (merged to 'next' on 2018-10-19 at 6db9b14495) + rebase -i: introduce the 'break' command + rebase -i: clarify what happens on a failed `exec` (this branch is used by js/rebase-i-shortopt; uses ag/rebase-i-in-c; is tangled with ag/sequencer-reduce-rewriting-todo, cb/printf-empty-format, js/rebase-autostash-fix, js/rebase-in-c-5.5-work-with-rebase-i-in-c and pk/rebase-in-c-6-final.) "git rebase -i" learned a new insn, 'break', that the user can insert in the to-do list. Upon hitting it, the command returns control back to the user. * js/rebase-i-shortopt (2018-10-26) 1 commit (merged to 'next' on 2018-11-01 at 4e9da19145) + rebase -i: recognize short commands without arguments (this branch uses ag/rebase-i-in-c and js/rebase-i-break; is tangled with
[PATCH 2+3/3] stash: tolerate missing user identity
The "git stash" command insists on having a usable user identity to
the same degree as the "git commit-tree" and "git commit" commands
do, because it uses the same codepath that creates commit objects
as these commands.
It is not strictly necesary to do so. Check if we will barf before
creating commit objects and then supply fake identity to please the
machinery that creates commits.
This is not that much of usability improvement, as the users who run
"git stash" would eventually want to record their changes that are
temporarily stored in the stashes in a more permanent history by
committing, and they must do "git config user.{name,email}" at that
point anyway, so arguably this change is only delaying a step that
is necessary to work in the repository.
Signed-off-by: Junio C Hamano
---
* This time with a proposed commit log message and a flip to the
test; this would be able to replce 2/3 and 3/3 without waiting
for ps/stash-in-c to stabilize and become ready to be based on
further work like this one.
We need to extend the test so that when a reasonable identity is
present, the stashes are created under that identity and not with
the fallback one, which I do not think is tested with the previous
step, so there still is a bit of room to improve [PATCH 1/3]
git-stash.sh | 17 +
t/t3903-stash.sh | 2 +-
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/git-stash.sh b/git-stash.sh
index 94793c1a91..789ce2f41d 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -55,6 +55,20 @@ untracked_files () {
git ls-files -o $z $excl_opt -- "$@"
}
+prepare_fallback_ident () {
+ if ! git -c user.useconfigonly=yes var GIT_COMMITTER_IDENT >/dev/null
2>&1
+ then
+ GIT_AUTHOR_NAME="git stash"
+ GIT_AUTHOR_EMAIL=git@stash
+ GIT_COMMITTER_NAME="git stash"
+ GIT_COMMITTER_EMAIL=git@stash
+ export GIT_AUTHOR_NAME
+ export GIT_AUTHOR_EMAIL
+ export GIT_COMMITTER_NAME
+ export GIT_COMMITTER_EMAIL
+ fi
+}
+
clear_stash () {
if test $# != 0
then
@@ -67,6 +81,9 @@ clear_stash () {
}
create_stash () {
+
+ prepare_fallback_ident
+
stash_msg=
untracked=
while test $# != 0
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index a9a573efa0..3dcf2f14d1 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1096,7 +1096,7 @@ test_expect_success 'stash -- works with binary
files' '
test_path_is_file subdir/untracked
'
-test_expect_failure 'stash works when user.name and user.email are not set' '
+test_expect_success 'stash works when user.name and user.email are not set' '
git reset &&
>1 &&
git add 1 &&
--
2.19.1-801-gd582ea202b
Re: [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function
Junio C Hamano writes:
> Rather than adding this fallback trap, can't we do it more like
> this?
>
> - At the beginning of "git stash", after parsing the command
> line, we know what subcommand of "git stash" we are going to
> run.
>
> - If it is a subcommand that could need the ident (i.e. the ones
> that create a stash entry), we check the ident (e.g. make a
> call to git_author/committer_info() ourselves) but without
> STRICT bit, so that we can probe without dying if we need to
> supply a fallback identy.
>
> - And if we do need it, then setenv() the necessary
> environment variables and arrange the next call by anybody
> to git_author/committer_info() will get the fallback values
> from there.
As we currently have no idea when builtin/stash.c becomes ready for
'next', how about doing something like this instead, in order to
help end-users without waiting in the meantime? The fix can be
picked up and ported when the C rewrite is updated, of course.
git-stash.sh | 17 +
1 file changed, 17 insertions(+)
diff --git a/git-stash.sh b/git-stash.sh
index 94793c1a91..789ce2f41d 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -55,6 +55,20 @@ untracked_files () {
git ls-files -o $z $excl_opt -- "$@"
}
+prepare_fallback_ident () {
+ if ! git -c user.useconfigonly=yes var GIT_COMMITTER_IDENT >/dev/null
2>&1
+ then
+ GIT_AUTHOR_NAME="git stash"
+ GIT_AUTHOR_EMAIL=git@stash
+ GIT_COMMITTER_NAME="git stash"
+ GIT_COMMITTER_EMAIL=git@stash
+ export GIT_AUTHOR_NAME
+ export GIT_AUTHOR_EMAIL
+ export GIT_COMMITTER_NAME
+ export GIT_COMMITTER_EMAIL
+ fi
+}
+
clear_stash () {
if test $# != 0
then
@@ -67,6 +81,9 @@ clear_stash () {
}
create_stash () {
+
+ prepare_fallback_ident
+
stash_msg=
untracked=
while test $# != 0
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
Ævar Arnfjörð Bjarmason writes:
> Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON
> test parameter to only be a GIT_TEST_GETTEXT_POISON=
> runtime parameter, to be consistent with other parameters documented
> in "Running tests with special setups" in t/README.
> ...
> #ifndef NO_GETTEXT
> extern void git_setup_gettext(void);
> extern int gettext_width(const char *s);
> #else
> static inline void git_setup_gettext(void)
> {
> + use_gettext_poison();; /* getenv() reentrancy paranoia */
> }
Too many "case/esac" statement in shell scripting?
Re: git appears to ignore GIT_CONFIG environment variable
Sirio Balmelli writes: > It appears that git ignores the GIT_CONFIG environment variable, > while git-config *does* consider it. Yup, that is exactly how it is designed and documented. These dasys, with "git config" taking "--file" to work on any arbitrary filename, you do not necessarily need GIT_CONFIG enviornment.
Re: [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function
Slavica Djukic writes:
> Usually, when creating a commit, ident is needed to record the author
> and commiter.
> But, when there is commit not intended to published, e.g. when stashing
> changes, valid ident is not necessary.
> To allow creating commits in such scenario, let's introduce helper
> function "set_fallback_ident(), which will pre-load the ident.
>
> In following commit, set_fallback_ident() function will be called in stash.
>
> Signed-off-by: Slavica Djukic
> ---
This is quite the other way around from what I expected to see.
I would have expected that a patch would introduce a new flag to
tell git_author/committer_info() functions that it is OK not to have
name or email given, and pass that flag down the callchain.
Anybody who knows that git_author/committer_info() will eventually
get called can instead tell these helpers not to fail (and yield a
substitute non-answer) beforehand with this function, instead of
passing a flag down to affect _only_ one callflow without affecting
others, using this new function.
I am not yet saying that being opposite from my intuition is
necessarily wrong, but the approach is like setting a global
variable that affects everybody and it will probably make it
harder to later libify the functions involved. It certainly
makes this patch (and the next step) much simpler than passing
a flag IDENT_NO_NAME_OK|IDENT_NO_MAIL_OK thru the codepath.
> +void set_fallback_ident(const char *name, const char *email)
> +{
> + if (!git_default_name.len) {
> + strbuf_addstr(_default_name, name);
> + committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
> + author_ident_explicitly_given |= IDENT_NAME_GIVEN;
> + ident_config_given |= IDENT_NAME_GIVEN;
> + }
> +
> + if (!git_default_email.len) {
> + strbuf_addstr(_default_email, email);
> + committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
> + author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
> + ident_config_given |= IDENT_MAIL_GIVEN;
> + }
> +}
One terrible thing about this approach is that the caller cannot
tell if it used fallback name or a real name, because it lies in
these "explicitly_given" fields. The immediate caller (i.e. the one
that creates commit objects used to represent a stash entry) may not
care, but a helper function pretending to be reusable incredient to
solve a more general issue, this is far less than ideal.
So in short, I do agree that the series tackles an issue worth
addressing, but I am not impressed with the approach at all.
Rather than adding this fallback trap, can't we do it more like
this?
- At the beginning of "git stash", after parsing the command
line, we know what subcommand of "git stash" we are going to
run.
- If it is a subcommand that could need the ident (i.e. the ones
that create a stash entry), we check the ident (e.g. make a
call to git_author/committer_info() ourselves) but without
STRICT bit, so that we can probe without dying if we need to
supply a fallback identy.
- And if we do need it, then setenv() the necessary
environment variables and arrange the next call by anybody
to git_author/committer_info() will get the fallback values
from there.
Re: [PATCH] travis-ci: install packages in 'ci/install-dependencies.sh'
SZEDER Gábor writes: > Ever since we started using Travis CI, we specified the list of > packages to install in '.travis.yml' via the APT addon. While running > our builds on Travis CI's container-based infrastructure we didn't > have another choice, because that environment didn't support 'sudo', > and thus we didn't have permission to install packages ourselves. With > the switch to the VM-based infrastructure in the previous patch we do > get a working 'sudo', so we can install packages by running 'sudo > apt-get -y install ...' as well. OK, so far we learned that this is now _doable_; but not enough to decide if this is a good thing to do or not. Let's read on to find out. > Let's make use of this and install necessary packages in > 'ci/install-dependencies.sh', so all the dependencies (i.e. both > packages and "non-packages" (P4 and Git-LFS)) are handled in the same > file. So we used to have two waysto prepare the test environment; non packaged software were done via install-dependencies.sh, but packaged ones weren't. Unifying them so that the script installs both would be a good change to simplify the procedure. Is that how this sentence argues for this change? > Install gcc-8 only in the 'linux-gcc' build job; so far it has > been unnecessarily installed in the 'linux-clang' build job as well. Is this "unneeded gcc-8 was installed" something we can fix only because we now stopped doing the installation via apt addon? Or we could have fixed it while we were on apt addon but we didn't bother, and this patch fixes it "while at it"---primarily because the shell script is far more flexible to work with than travis.yml matrix and this kind of customization is far easier to do? > Print the versions of P4 and Git-LFS conditionally, i.e. only when > they have been installed; with this change even the static analysis > and documentation build jobs start using 'ci/install-dependencies.sh' > to install packages, and neither of these two build jobs depend on and > thus install those. > > This change will presumably be beneficial for the upcoming Azure > Pipelines integration [1]: preliminary versions of that patch series > run a couple of 'apt-get' commands to install the necessary packages > before running 'ci/install-dependencies.sh', but with this patch it > will be sufficient to run only 'ci/install-dependencies.sh'. So the main point of this change is to have less knowledge to prepare the target configuration in the .travis.yml file and keep them all in ci/install-dependencies.sh, which hopefully is more reusable than .travis.yml in a non Travis environment? If that is the case, it makes sense to me. > This patch should go on top of 'ss/travis-ci-force-vm-mode'. > > I'm not sure about the last paragraph, because: > > - It talks about presumed benefits for a currently still > work-in-progress patch series of an other contributor, and I'm not > really sure that that's a good thing. Perhaps I should have > rather put it below the '---'. > > - I'm confused about the name of this Azure thing. The cover letter > mentions "Azure Pipelines", the file is called > 'azure-pipelines.yml', but the relevant patch I link to talks > about "Azure DevOps" in the commit message. > > Anyway, keep that last paragraph or drop it as you see fit. I hope we'll hear from Dscho in one or two revolutions of the Earth ;-) > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh > index 75a9fd2475..06c3546e1e 100755 > --- a/ci/install-dependencies.sh > +++ b/ci/install-dependencies.sh > @@ -10,6 +10,15 @@ > LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VE > > case "$jobname" in > linux-clang|linux-gcc) > + sudo apt-add-repository -y "ppa:ubuntu-toolchain-r/test" > + sudo apt-get -q update > + sudo apt-get -q -y install language-pack-is git-svn apache2 > + case "$jobname" in > + linux-gcc) > + sudo apt-get -q -y install gcc-8 > + ;; > + esac > + > mkdir --parents "$P4_PATH" > pushd "$P4_PATH" > wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d" > @@ -32,11 +41,25 @@ osx-clang|osx-gcc) > brew link --force gettext > brew install caskroom/cask/perforce > ;; > +StaticAnalysis) > + sudo apt-get -q update > + sudo apt-get -q -y install coccinelle > + ;; > +Documentation) > + sudo apt-get -q update > + sudo apt-get -q -y install asciidoc xmlto > + ;; > esac > > -echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)" > -p4d -V | grep Rev. > -echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)" > -p4 -V | grep Rev. > -echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)" > -git-lfs version > +if type p4d >/dev/null && type p4 >/dev/null > +then > + echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)" > + p4d -V | grep Rev. > + echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)" > + p4 -V | grep Rev.
Re: [PATCHv2 00/24] Bring more repository handles into our code base]
Stefan Beller writes: >> What was posted would have been perfectly fine as a "how about doing >> it this way" weatherbaloon patch, but as a part of real series, it >> needs to explain to the developers what the distinctions between two >> classes are, and who is to use the cocci patch at what point in the >> development cycle, etc., in an accompanying documentation update. > > if only we had documentation [as found via "git grep coccicheck"] > that I could update ... I'd totally do that. > I guess this asks for documentation to begin with, now? So far, we didn't even need any, as there was no "workflow" to speak of. It's just "any random developer finds a suggested update, either by running 'make coccicheck' oneself or by peeking Travis log, and sends in a patch". But the "pending" thing has a lot more workflow elements, who is responsible for noticing update suggested by "pending" ones, for updating the code, and for promoting "pending" to the normal. These are all new, and these are all needed as ongoing basis to help developers---I'd say the way Documentation/SubmittingPatches helps developers is very close to the way this new document would help them.
Re: [PATCH 1/3] commit-reach: implement get_reachable_subset
Derrick Stolee writes: > We could do this, but it does come with a performance hit when the following > are all true: > > 1. 'to' is not reachable from any 'from' commits. > > 2. The 'to' and 'from' commits are close in commit-date. > > 3. Generation numbers are not available, or the topology is skewed to have >commits with high commit date and low generation number. > > Since in_merge_bases_many() calls paint_down_to_common(), it has the same > issues with the current generation numbers. This can be fixed when we have > the next version of generation numbers available. > > I'll make a note to have in_merge_bases_many() call get_reachable_subset() > conditionally (like the generation_numbers_available() trick in the > --topo-order > series) after the generation numbers are settled and implemented. Sounds good. I wondered how this compares with in_merge_bases_many() primarily because how performance characteristics between two approaches trade off. After all, what it needs to compute is a specialized case of get_reachable_subset() where "to" happens to be a set with only single element. If the latter with a single element "to" has the above performance "issues" compared to paint-down-to-common based approach, it could be possible that, for small enough N>1, running N in_merge_bases_many() traversals is more performant than a single get_reachable_subset() call that works on N-element "to". I am hoping that an update to better generation numbers to help get_reachable_subset() would help paint-down-to-common the same way, and would eventually allow us to use a single traversal that is best for N==1 and N>1. Thanks.
Re: master updated? (was Re: What's cooking in git.git (Nov 2018, #01; Thu, 1))
Derrick Stolee writes: > On 11/1/2018 5:59 AM, Junio C Hamano wrote: >> -- >> [Graduated to "master"] > > I see that several topics graduated, but it appears the master branch > was not updated at https://github.com/gister/git. Was this > intentional? The "Nov #1" issue of "What's cooking" report lists various topics including ah/doc-updates, ..., uk/merge- subtree-doc-update in the "Graduated" section by mistake (probably due to crossing month boundary); they already were listed in the previous "Oct #6" issue. Hopefully we'll have the following topics graduate to 'master' today, as part of the Eighth batch of the cycle. js/rebase-i-shortopt js/rebase-i-break js/rebase-autostash-fix cb/printf-empty-format jc/rebase-in-c-5-test-typofix pk/rebase-in-c-6-final js/rebase-in-c-5.5-work-with-rebase-i-in-c pk/rebase-in-c-5-test pk/rebase-in-c-4-opts pk/rebase-in-c-3-acts pk/rebase-in-c-2-basic ag/rebase-i-in-c pk/rebase-in-c Thanks.
Re: [PATCH v4 0/7] Use generation numbers for --topo-order
Derrick Stolee writes: >> Review discussions seem to have petered out. Would we merge this to >> 'next' and start cooking, perhaps for the remainder of this cycle? > > Thanks, but I've just sent a v5 responding to Jakub's feedback on v4. [1] > > I'd be happy to let it sit in next until you feel it has cooked long > enough. I'm available to respond to feedback in the form of new > topics. OK. I'm quite happy to see this round of review helped greatly by Jakub, by the way. THanks, both.
Re: [PATCH v2] completion: use builtin completion for format-patch
Duy Nguyen writes: >> > I have no comment about this. In an ideal world, sendemail.perl could >> > be taught to support --git-completion-helper but I don't think my >> > little remaining Perl knowledge (or time) is enough to do it. Perhaps >> > this will do. I don't know. >> >> So "all", "attach", etc. are added to this list while these similar >> options are lost from the other variable? Is this a good trade-off? > > Not sure if I understand you correctly, but it looks to me that the > options in git-send-email.perl are well organized, so we could... Yes, but I wasn't commenting on your "sendemail should also be able to help completion by supporting --completion-helper option" (I think that is a sensible approach). My comment was about Denton's patch, which reduced the hard-coded list of format-patch options (i.e. the first hunk) but had to add back many of them to send-email's completion (i.e. the last hunk)---overall, it did not help reducing the number of options hardcoded in the script. If it makes sense to complete all options to format-patch to send-email, then as you outlined, grabbing them out of format-patch with the --completion-helper option at runtime, and using them to complete both format-patch and send-email would be a good idea. And that should be doable even before send-email learns how to list its supported options to help the completion. > --git-completon-helper in that script to print all send-email specific > options, then call "git format-patch --git-completion-helper" to add a > bunch more. The options that are handled by setup_revisions() will > have to be maintained manually here like $__git_format_patch_options > and added on top in both _git_send_email () and _git_format_patch (). > > So, nothing option is lost and by the time setup_revisions() supports > -git-completion-helper, we can get rid of the manual shell variable > too. The downside is, lots of work, probably.
Re: [PATCH 3/3] tests: optionally skip `git rebase -p` tests
Johannes Sixt writes: > Am 01.11.18 um 07:12 schrieb Junio C Hamano: >> "Johannes Schindelin via GitGitGadget" >> writes: >> >>> The `--preserve-merges` mode of the `rebase` command is slated to be >>> deprecated soon, ... >> >> Is everybody on board on this statement? I vaguely recall that some >> people wanted to have something different from what rebase-merges >> does (e.g. wrt first-parent history), and extending perserve-merges >> might be one way to do so. > > Maybe you are referring to my proposals from a long time ago. My > first-parent hack did not work very well, and I have changed my > workflow. --preserve-merges is certainly not a feature that *I* would > like to keep. Thanks, that reduces my worries. > The important question is whether there are too many users of > preserve-merges who would be hurt when it is removed. Yes, and the claim this series makes is that there is none and all existing users should be able to happily use the rebase-merges, which also means that we need to commit to improve rebase-merges to support them, if there were some corner cases, which we failed to consider so far, that are not yet served well. As I said, as long as everybody agrees with the plan (e.g. we'll know when we hear no objections to the planned deprecation in a few weeks), I am perfectly OK with it. Thanks.
Re: ab/* topics
Ævar Arnfjörð Bjarmason writes: > Could you please pick up > https://public-inbox.org/git/20181024114725.3927-1-ava...@gmail.com/ ? > It seems to have fallen between the cracks and addressed the feedback on > v1, and looks good to me (and nobody's objected so far...). If this is the runtime-gettext-poison thing, this did not fall thru the cracks---I didn't get the impression that "no objection was a sign of being excellent"; rather I recall feeling that you were the only person who were excited about it, while everybody else was "Meh". Thanks for pinging. It is very possible that I didn't read (or rememer) the thread correctly. Let me go back to the archive in the morning to double check.
What's cooking in git.git (Nov 2018, #01; Thu, 1)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. Two groups of "rewrite rebase/rebase-i in C" topics, together with a handful of associated fix-up topics to them, will all be merged to 'master' tomorrow. Some of them haven't spent as much time as usual in 'next', so there still may be rough edges, but let's make sure we find them and smooth them out before the release. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * ah/doc-updates (2018-10-23) 6 commits (merged to 'next' on 2018-10-26 at b0bb46a602) + doc: fix formatting in git-update-ref + doc: fix indentation of listing blocks in gitweb.conf.txt + doc: fix descripion for 'git tag --format' + doc: fix inappropriate monospace formatting + doc: fix ASCII art tab spacing + doc: clarify boundaries of 'git worktree list --porcelain' Doc updates. * bc/hash-transition-part-15 (2018-10-15) 15 commits (merged to 'next' on 2018-10-26 at 4ff8111b4b) + rerere: convert to use the_hash_algo + submodule: make zero-oid comparison hash function agnostic + apply: rename new_sha1_prefix and old_sha1_prefix + apply: replace hard-coded constants + tag: express constant in terms of the_hash_algo + transport: use parse_oid_hex instead of a constant + upload-pack: express constants in terms of the_hash_algo + refs/packed-backend: express constants using the_hash_algo + packfile: express constants in terms of the_hash_algo + pack-revindex: express constants in terms of the_hash_algo + builtin/fetch-pack: remove constants with parse_oid_hex + builtin/mktree: remove hard-coded constant + builtin/repack: replace hard-coded constants + pack-bitmap-write: use GIT_MAX_RAWSZ for allocation + object_id.cocci: match only expressions of type 'struct object_id' More codepaths are moving away from hardcoded hash sizes. * cb/compat-mmap-is-private-read-only (2018-10-25) 1 commit (merged to 'next' on 2018-10-26 at d3bfab3034) + compat: make sure git_mmap is not expected to write Code tightening. * cb/khash-maybe-unused-function (2018-10-24) 2 commits (merged to 'next' on 2018-10-26 at 17fc4e55a0) + khash: silence -Wunused-function for delta-islands + commit-slabs: move MAYBE_UNUSED out Build fix. * cb/remove-dead-init (2018-10-19) 2 commits (merged to 'next' on 2018-10-26 at ba725a64ad) + multi-pack-index: avoid dead store for struct progress + unpack-trees: avoid dead store for struct progress Code clean-up. * ch/subtree-build (2018-10-18) 3 commits (merged to 'next' on 2018-10-18 at f89fd5e6aa) + Revert "subtree: make install targets depend on build targets" (merged to 'next' on 2018-10-16 at 919599cc37) + subtree: make install targets depend on build targets (merged to 'next' on 2018-10-12 at 4ed9ff6300) + subtree: add build targets 'man' and 'html' Build update for "git subtree" (in contrib/) documentation pages. * dl/mergetool-gui-option (2018-10-25) 3 commits (merged to 'next' on 2018-10-26 at 2c46355e81) + doc: document diff/merge.guitool config keys + completion: support `git mergetool --[no-]gui` + mergetool: accept -g/--[no-]gui as arguments "git mergetool" learned to take the "--[no-]gui" option, just like "git difftool" does. * ds/ci-commit-graph-and-midx (2018-10-19) 1 commit (merged to 'next' on 2018-10-26 at a13664e49a) + ci: add optional test variables One of our CI tests to run with "unusual/experimental/random" settings now also uses commit-graph and midx. * ds/reachable (2018-10-23) 1 commit (merged to 'next' on 2018-10-26 at 76b5fc9a46) + commit-reach: fix cast in compare_commits_by_gen() Trivial bugfix. * ds/reachable-first-parent-fix (2018-10-19) 1 commit (merged to 'next' on 2018-10-26 at 076442d512) + commit-reach: fix first-parent heuristic Correct performance regression in commit ancestry computation when generation numbers are involved. * jc/cocci-preincr (2018-10-24) 2 commits (merged to 'next' on 2018-10-26 at cbd98b44e2) + fsck: s/++i > 1/i++/ + cocci: simplify "if (++u > 1)" to "if (u++)" Code cleanup. * jc/receive-deny-current-branch-fix (2018-10-19) 1 commit (merged to 'next' on 2018-10-26 at 2975c42215) + receive: denyCurrentBranch=updateinstead should not blindly update The receive.denyCurrentBranch=updateInstead codepath kicked in even when the push should have been rejected due to other reasons, such as it does not fast-forward or the update-hook rejects it, which has been corrected. * jk/run-command-notdot (2018-10-25) 2 commits (merged to 'next' on 2018-10-26 at 9d9335b23f) + t0061: adjust to test-tool transition + run-command:
Re: [PATCH 3/3] tests: optionally skip `git rebase -p` tests
"Johannes Schindelin via GitGitGadget" writes: > The `--preserve-merges` mode of the `rebase` command is slated to be > deprecated soon, ... Is everybody on board on this statement? I vaguely recall that some people wanted to have something different from what rebase-merges does (e.g. wrt first-parent history), and extending perserve-merges might be one way to do so. I do not mind at all if the way forward were to extend rebase-merges for any future features. To me, it is preferrable having to deal with just one codepath than two written in different languages. I just want to make sure we know everybody is on board the plan that we will eventually remove preserve-merges, tell those who want to use it to switch to rebase-merges, and we will extend rebase-merges when they raise issues with it saying that they cannot do something preserve-merges would have served them well with rebase-merges.
Re: [PATCH v4 10/12] Add a base implementation of SHA-256 support
"brian m. carlson" writes: > I need to do a reroll to address some other issues people brought up, so > I can remove this line. OK. I actually do not feel too strongly about this one, by the way.
Re: [PATCH v4 0/7] Use generation numbers for --topo-order
"Derrick Stolee via GitGitGadget" writes: > This patch series performs a decently-sized refactoring of the revision-walk > machinery. Well, "refactoring" is probably the wrong word, as I don't > actually remove the old code. Instead, when we see certain options in the > 'rev_info' struct, we redirect the commit-walk logic to a new set of methods > that distribute the workload differently. By using generation numbers in the > commit-graph, we can significantly improve 'git log --graph' commands (and > the underlying 'git rev-list --topo-order'). Review discussions seem to have petered out. Would we merge this to 'next' and start cooking, perhaps for the remainder of this cycle?
Re: [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:"
Æ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).
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.
Re: [PATCH 2/3] tests: mark those tests where "git fsck" fails at the end
Ævar Arnfjörð Bjarmason writes:
> Mark the tests where "git fsck" fails at the end with extra test code
> to check the fsck output. There fsck.{err,out} has been created for
> us.
>
> A later change will add the support for GIT_TEST_FSCK_TESTS. They're
> being added first to ensure the test suite will never fail with
> GIT_TEST_FSCK=true during bisect.
I am sympathetic to what step 3/3 (eh, rather, an earlier "let's not
leave the repository in corrupt state, as that would make it
inconvenient for us to later append new tests") wants to do, but not
this one---these markings at the end makes it inconvenient for us to
later add new tests to these script before them.
[PATCH v4fixed 4/5] add read_author_script() to libgit
From: Phillip Wood
Add read_author_script() to sequencer.c based on the implementation in
builtin/am.c and update read_am_author_script() to use
read_author_script(). The sequencer code that reads the author script
will be updated in the next commit.
Signed-off-by: Phillip Wood
Signed-off-by: Junio C Hamano
---
builtin/am.c | 86 +
sequencer.c | 105 +++
sequencer.h | 3 ++
3 files changed, 110 insertions(+), 84 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index c78a745289..83685180e0 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -260,36 +260,6 @@ static int read_state_file(struct strbuf *sb, const struct
am_state *state,
die_errno(_("could not read '%s'"), am_path(state, file));
}
-/**
- * Take a series of KEY='VALUE' lines where VALUE part is
- * sq-quoted, and append at the end of the string list
- */
-static int parse_key_value_squoted(char *buf, struct string_list *list)
-{
- while (*buf) {
- struct string_list_item *item;
- char *np;
- char *cp = strchr(buf, '=');
- if (!cp) {
- np = strchrnul(buf, '\n');
- return error(_("unable to parse '%.*s'"),
-(int) (np - buf), buf);
- }
- np = strchrnul(cp, '\n');
- *cp++ = '\0';
- item = string_list_append(list, buf);
-
- buf = np + (*np == '\n');
- *np = '\0';
- cp = sq_dequote(cp);
- if (!cp)
- return error(_("unable to dequote value of '%s'"),
-item->string);
- item->util = xstrdup(cp);
- }
- return 0;
-}
-
/**
* Reads and parses the state directory's "author-script" file, and sets
* state->author_name, state->author_email and state->author_date accordingly.
@@ -309,65 +279,13 @@ static int parse_key_value_squoted(char *buf, struct
string_list *list)
static int read_am_author_script(struct am_state *state)
{
const char *filename = am_path(state, "author-script");
- struct strbuf buf = STRBUF_INIT;
- struct string_list kv = STRING_LIST_INIT_DUP;
- int retval = -1; /* assume failure */
- int i, name_i = -2, email_i = -2, date_i = -2, err = 0;
- int fd;
assert(!state->author_name);
assert(!state->author_email);
assert(!state->author_date);
- fd = open(filename, O_RDONLY);
- if (fd < 0) {
- if (errno == ENOENT)
- return 0;
- return error_errno(_("could not open '%s' for reading"),
- filename);
- }
- strbuf_read(, fd, 0);
- close(fd);
- if (parse_key_value_squoted(buf.buf, ))
- goto finish;
-
- for (i = 0; i < kv.nr; i++) {
- if (!strcmp(kv.items[i].string, "GIT_AUTHOR_NAME")) {
- if (name_i != -2)
- name_i = error(_("'GIT_AUTHOR_NAME' already
given"));
- else
- name_i = i;
- } else if (!strcmp(kv.items[i].string, "GIT_AUTHOR_EMAIL")) {
- if (email_i != -2)
- email_i = error(_("'GIT_AUTHOR_EMAIL' already
given"));
- else
- email_i = i;
- } else if (!strcmp(kv.items[i].string, "GIT_AUTHOR_DATE")) {
- if (date_i != -2)
- date_i = error(_("'GIT_AUTHOR_DATE' already
given"));
- else
- date_i = i;
- } else {
- err = error(_("unknown variable '%s'"),
- kv.items[i].string);
- }
- }
- if (name_i == -2)
- error(_("missing 'GIT_AUTHOR_NAME'"));
- if (email_i == -2)
- error(_("missing 'GIT_AUTHOR_EMAIL'"));
- if (date_i == -2)
- error(_("missing 'GIT_AUTHOR_DATE'"));
- if (date_i < 0 || email_i < 0 || date_i < 0 || err)
- goto finish;
- state->author_name = kv.items[name_i].util;
- state->author_email = kv.items[email_i].util;
- state->author_date = kv.items[date_i].util;
- retval = 0;
-finish:
- string_list_clear(, !!retval);
- strbuf_release();
- return retval;
+ return read_author_script(filename, >author_name,
+ >author_email, >author_date, 1);
}
/**
diff --git a/sequencer.c b/sequencer.c
ind
[PATCH v4fixed 2/5] am: improve author-script error reporting
From: Phillip Wood
If there are errors in a user edited author-script there was no
indication of what was wrong. This commit adds some specific error messages
depending on the problem. It also relaxes the requirement that the
variables appear in a specific order in the file to match the behavior
of 'rebase --interactive'.
Signed-off-by: Phillip Wood
Signed-off-by: Junio C Hamano
---
builtin/am.c | 49 +++--
1 file changed, 39 insertions(+), 10 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 4f7f28a9dc..ffca4479d7 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -270,8 +270,11 @@ static int parse_key_value_squoted(char *buf, struct
string_list *list)
struct string_list_item *item;
char *np;
char *cp = strchr(buf, '=');
- if (!cp)
- return -1;
+ if (!cp) {
+ np = strchrnul(buf, '\n');
+ return error(_("unable to parse '%.*s'"),
+(int) (np - buf), buf);
+ }
np = strchrnul(cp, '\n');
*cp++ = '\0';
item = string_list_append(list, buf);
@@ -280,7 +283,8 @@ static int parse_key_value_squoted(char *buf, struct
string_list *list)
*np = '\0';
cp = sq_dequote(cp);
if (!cp)
- return -1;
+ return error(_("unable to dequote value of '%s'"),
+item->string);
item->util = xstrdup(cp);
}
return 0;
@@ -308,6 +312,7 @@ static int read_author_script(struct am_state *state)
struct strbuf buf = STRBUF_INIT;
struct string_list kv = STRING_LIST_INIT_DUP;
int retval = -1; /* assume failure */
+ int i, name_i = -2, email_i = -2, date_i = -2, err = 0;
int fd;
assert(!state->author_name);
@@ -326,14 +331,38 @@ static int read_author_script(struct am_state *state)
if (parse_key_value_squoted(buf.buf, ))
goto finish;
- if (kv.nr != 3 ||
- strcmp(kv.items[0].string, "GIT_AUTHOR_NAME") ||
- strcmp(kv.items[1].string, "GIT_AUTHOR_EMAIL") ||
- strcmp(kv.items[2].string, "GIT_AUTHOR_DATE"))
+ for (i = 0; i < kv.nr; i++) {
+ if (!strcmp(kv.items[i].string, "GIT_AUTHOR_NAME")) {
+ if (name_i != -2)
+ name_i = error(_("'GIT_AUTHOR_NAME' already
given"));
+ else
+ name_i = i;
+ } else if (!strcmp(kv.items[i].string, "GIT_AUTHOR_EMAIL")) {
+ if (email_i != -2)
+ email_i = error(_("'GIT_AUTHOR_EMAIL' already
given"));
+ else
+ email_i = i;
+ } else if (!strcmp(kv.items[i].string, "GIT_AUTHOR_DATE")) {
+ if (date_i != -2)
+ date_i = error(_("'GIT_AUTHOR_DATE' already
given"));
+ else
+ date_i = i;
+ } else {
+ err = error(_("unknown variable '%s'"),
+ kv.items[i].string);
+ }
+ }
+ if (name_i == -2)
+ error(_("missing 'GIT_AUTHOR_NAME'"));
+ if (email_i == -2)
+ error(_("missing 'GIT_AUTHOR_EMAIL'"));
+ if (date_i == -2)
+ error(_("missing 'GIT_AUTHOR_DATE'"));
+ if (date_i < 0 || email_i < 0 || date_i < 0 || err)
goto finish;
- state->author_name = kv.items[0].util;
- state->author_email = kv.items[1].util;
- state->author_date = kv.items[2].util;
+ state->author_name = kv.items[name_i].util;
+ state->author_email = kv.items[email_i].util;
+ state->author_date = kv.items[date_i].util;
retval = 0;
finish:
string_list_clear(, !!retval);
--
2.19.1-708-g4ede3d42df
Re: [PATCH v4 0/5] am/rebase: share read_author_script()
Phillip Wood writes: > From: Phillip Wood > > Sorry for the confusion with v3, here are the updated patches. > > Thanks to Junio for the feedback on v2. I've updated patch 4 based on > those comments, the rest are unchanged. The mistake of overwriting -1 (i.e. earlier we detected dup) with the third instance of the same originates at [2/5], so updating [4/5] without fixing it at its source would mean [4/5] is not a pure code movement to make it available to libgit users---it instead hides a (not so important) bugfix in it. > > v1 cover letter: > > This is a follow up to pw/rebase-i-author-script-fix, it reduces code > duplication and improves rebase's parsing of the author script. After > this I'll do another series to share the code to write the author > script. > > > Phillip Wood (5): > am: don't die in read_author_script() > am: improve author-script error reporting > am: rename read_author_script() > add read_author_script() to libgit > sequencer: use read_author_script() > > builtin/am.c | 60 ++-- > sequencer.c | 192 --- > sequencer.h | 3 + > 3 files changed, 128 insertions(+), 127 deletions(-)
