Re: [PATCH/RFC] commit: add short option for --amend
On Thu, Aug 16, 2018 at 08:31:17PM +0200, Nguyễn Thái Ngọc Duy wrote: > I just realized how often I type "git ci --amend". Looking back at my > ~/.bash_history (only 10k lines) this is the second most often git > command I type which may justify a short option for it (assuming that > other people use this option often too, of course). Why not add another alias? As you're already using the ci alias, maybe cia? Personally I have the following aliases for committing: c = commit --verbose ca = commit --verbose --amend cad = commit --verbose --amend --date=now Besides the obvious g=git alias in the shell. I really like one character aliases for often used commands/subcommands. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: Git clone and case sensitivity
On Sat, Jul 28, 2018 at 07:11:05AM +0200, Duy Nguyen wrote:
> static int checkout(int submodule_progress)
> {
> struct object_id oid;
> @@ -761,6 +785,11 @@ static int checkout(int submodule_progress)
> if (write_locked_index(_index, _file, COMMIT_LOCK))
> die(_("unable to write new index file"));
>
> + if (ignore_case && has_duplicate_icase_entries(_index))
> + warning(_("This repository has paths that only differ in case\n"
> + "and you have a case-insenitive filesystem which
> will\n"
> + "cause problems."));
> +
> err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
> oid_to_hex(), "1", NULL);
I think the advice message should list the problematic file
names. Even though this might be quite verbose it will help those
affected to quickly find the problematic files to either fix this
on their own or report to upstream (unless there's already an
easy way to find those files - if so it should be mentioned in
the message).
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH v3 04/11] rerere: mark strings for translation
On Sat, Jul 14, 2018 at 10:44:36PM +0100, Thomas Gummerer wrote: > 'git rerere' is considered a plumbing command and as such its output s/plumbing/porcelain/? Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH v1 01/25] structured-logging: design document
On Fri, Jul 13, 2018 at 04:55:57PM +, g...@jeffhostetler.com wrote:
> diff --git a/Documentation/technical/structured-logging.txt
> b/Documentation/technical/structured-logging.txt
> new file mode 100644
> index 000..794c614
> --- /dev/null
> +++ b/Documentation/technical/structured-logging.txt
> @@ -0,0 +1,816 @@
> [snip]
>
> +"event": "cmd_start"
> +---
> +
> +The "cmd_start" event is emitted when git starts when cmd_main() is
> +called. In addition to the F1 fields, it contains the following
> +fields (F2):
> +
> +"argv":
> +
> + is an array of the original command line arguments given to the
> +command (before git.c has a chance to remove the global options
> +before the verb.
Missing closing parentheses.
> [snip]
>
> + are the values of the corresponding
> +"slog.{detail,timers,aux}" config setting. Since these values
> +control optional SLOG features and filtering, these are present
> +to help post-processors know if an expected event did not happen
> +or was simply filtered out. (Described later)
Please write out "slog_detail, "slog_timers", etc. Using the
abbreviated forms makes searching a pain.
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"
On Thu, Jun 14, 2018 at 10:25:03PM -0400, Eric Sunshine wrote: > The Makefile tweak NO_ICONV is meant to allow Git to be built without > iconv in case iconv is not installed or is otherwise dysfunctional. > However, NO_ICONV's disabling of iconv is incomplete and can incorrectly > allow "-liconv" to slip into the linker flags when NEEDS_LIBICONV is > defined, which breaks the build when iconv is not installed. > > On some platforms, iconv lives directly in libc, whereas, on others it > resides in libiconv. For the latter case, NEEDS_LIBICONV instructs the > Makefile to add "-liconv" to the linker flags. config.mak.uname > automatically defines NEEDS_LIBICONV for platforms which require it. > The adding of "-liconv" is done unconditionally, despite NO_ICONV. > > Work around this problem by making NO_ICONV take precedence over > NEEDS_LIBICONV. > > Reported by: Mahmoud Al-Qudsi > Signed-off-by: Eric Sunshine > --- > > This patch is extra noisy due to the indentation change. Viewing it with > "git diff -w" helps. An alternative to re-indenting would have been to > "undefine NEEDS_LIBICONV", however, 'undefine' was added to GNU make in > 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided. Should we put the part about MacOS's make into the commit message? Seems like relevant information for future readers. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH v2] Use proper syntax for replaceables in command docs
On Thu, May 24, 2018 at 04:11:39PM -0400, Robert P. J. Day wrote: > diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt > index 37b96c545..f98b7c6ed 100644 > --- a/Documentation/git-cvsserver.txt > +++ b/Documentation/git-cvsserver.txt > @@ -22,7 +22,7 @@ cvspserver stream tcp nowait nobody /usr/bin/git-cvsserver > git-cvsserver pserver > Usage: > > [verse] > -'git-cvsserver' [options] [pserver|server] [ ...] > +'git-cvsserver' [] [pserver|server] [ ...] No space in front of "..." for consistency? Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 6/8] diff.c: decouple white space treatment from move detection algorithm
On Thu, May 17, 2018 at 12:46:51PM -0700, Stefan Beller wrote: > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt > index bb9f1b7cd82..7b2527b9a19 100644 > --- a/Documentation/diff-options.txt > +++ b/Documentation/diff-options.txt > @@ -292,6 +292,19 @@ dimmed_zebra:: > blocks are considered interesting, the rest is uninteresting. > -- > > +--color-moved-[no-]ignore-space-at-eol:: > + Ignore changes in whitespace at EOL when performing the move > + detection for --color-moved. > +--color-moved-[no-]ignore-space-change:: > + Ignore changes in amount of whitespace when performing the move > + detection for --color-moved. This ignores whitespace > + at line end, and considers all other sequences of one or > + more whitespace characters to be equivalent. > +--color-moved-[no-]ignore-all-space:: > + Ignore whitespace when comparing lines when performing the move > + detection for --color-moved. This ignores differences even if > + one line has whitespace where the other line has none. > + > --word-diff[=]:: > Show a word diff, using the to delimit changed words. > By default, words are delimited by whitespace; see Hello, I think it would be better to specify the options unabbreviated. Not being able to search the man page for "--color-moved-ignore-space-at-eol" or "--color-moved-no-ignore-space-at-eol" can be a major pain when looking for documentation. So maybe something like this instead: > +--color-moved-ignore-space-at-eol:: > +--color-moved-no-ignore-space-at-eol:: > + Ignore changes in whitespace at EOL when performing the move > + detection for --color-moved. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 23/41] upload-pack: replace use of several hard-coded constants
On Mon, Apr 23, 2018 at 11:39:33PM +, brian m. carlson wrote: > [snip] > > diff --git a/upload-pack.c b/upload-pack.c > index 4a82602be5..0858527c5b 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -450,7 +450,7 @@ static int get_common_commits(void) > break; > default: > got_common = 1; > - memcpy(last_hex, oid_to_hex(), 41); > + oid_to_hex_r(last_hex, ); > if (multi_ack == 2) > packet_write_fmt(1, "ACK %s common\n", > last_hex); > else if (multi_ack) > @@ -492,7 +492,7 @@ static int do_reachable_revlist(struct child_process *cmd, > "rev-list", "--stdin", NULL, > }; > struct object *o; > - char namebuf[42]; /* ^ + SHA-1 + LF */ > + char namebuf[GIT_MAX_HEXSZ + 2]; /* ^ + SHA-1 + LF */ I think this comment should be "^ + hash as hex + LF". > @@ -561,15 +561,17 @@ static int get_reachable_list(struct object_array *src, > struct child_process cmd = CHILD_PROCESS_INIT; > int i; > struct object *o; > - char namebuf[42]; /* ^ + SHA-1 + LF */ > + char namebuf[GIT_MAX_HEXSZ + 2]; /* ^ + SHA-1 + LF */ > + const unsigned hexsz = the_hash_algo->hexsz; Dito. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: Silly "git gc" UI issue.
On Thu, Apr 19, 2018 at 02:10:40PM +0900, Junio C Hamano wrote:
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index c6679cb2cd..872627eafe 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -38,7 +38,11 @@ int parse_opt_approxidate_cb(const struct option *opt,
> const char *arg,
> int parse_opt_expiry_date_cb(const struct option *opt, const char *arg,
>int unset)
> {
> - return parse_expiry_date(arg, (timestamp_t *)opt->value);
> + if (unset)
> + arg = "never";
> + if (parse_expiry_date(arg, (timestamp_t *)opt->value))
> + die("malformed expiration date '%s'", arg);
> + return 0;
> }
Should this error get translated?
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH/RFC 0/5] Keep all info in command-list.txt in git binary
On Thu, Apr 19, 2018 at 01:26:18PM +0200, SZEDER Gábor wrote: > On Thu, Apr 19, 2018 at 12:37 PM, Simon Ruderich wrote: >> This doesn't occur on a non-parallel build. > > It does occur in non-parallel builds, too. > > See: > > > https://public-inbox.org/git/cam0vkjkns+asvymse2fxzt8a8oqydnx3qo8mnw2juogfc7l...@mail.gmail.com/ Thanks for the correction. I noticed it at the beginning of make -j run and incorrectly assummed it would occur quickly in the non-parallel run as well. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH/RFC 0/5] Keep all info in command-list.txt in git binary
Hello, When running make -j$(nproc) with the current pu f9f8c1f765 (Merge branch 'hn/bisect-first-parent' into pu) I see the following error: GIT_VERSION = 2.17.0.732.gf9f8c1f765 * new build flags * new prefix flags GEN common-cmds.h * new link flags CC ident.o CC hex.o CC json-writer.o ./generate-cmdlist.sh: 73: ./generate-cmdlist.sh: Bad substitution This doesn't occur on a non-parallel build. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: Silly "git gc" UI issue.
On Thu, Apr 19, 2018 at 10:52:47AM +0900, Junio C Hamano wrote:
> It turns out that prune silently goes away given a bad expiry
>
> $ git prune --expire=nyah ; echo $?
> 129
I noticed that git log --since/--after/--before/--until have a
similar behavior and ignore date parsing errors in those options
completely. Is this expected or should we warn the user with
something like the following?
diff --git a/revision.c b/revision.c
index 4e0e193e57..e5ba6c7dfc 100644
--- a/revision.c
+++ b/revision.c
@@ -1794,19 +1794,31 @@ static int handle_revision_opt(struct rev_info *revs,
int argc, const char **arg
revs->max_age = atoi(optarg);
return argcount;
} else if ((argcount = parse_long_opt("since", argv, ))) {
- revs->max_age = approxidate(optarg);
+ int err = 0;
+ revs->max_age = approxidate_careful(optarg, );
+ if (err)
+ return error("--since: invalid time '%s'", optarg);
return argcount;
} else if ((argcount = parse_long_opt("after", argv, ))) {
- revs->max_age = approxidate(optarg);
+ int err = 0;
+ revs->max_age = approxidate_careful(optarg, );
+ if (err)
+ return error("--after: invalid time '%s'", optarg);
return argcount;
} else if ((argcount = parse_long_opt("min-age", argv, ))) {
revs->min_age = atoi(optarg);
return argcount;
} else if ((argcount = parse_long_opt("before", argv, ))) {
- revs->min_age = approxidate(optarg);
+ int err = 0;
+ revs->min_age = approxidate_careful(optarg, );
+ if (err)
+ return error("--before: invalid time '%s'", optarg);
return argcount;
} else if ((argcount = parse_long_opt("until", argv, ))) {
- revs->min_age = approxidate(optarg);
+ int err = 0;
+ revs->min_age = approxidate_careful(optarg, );
+ if (err)
+ return error("--until: invalid time '%s'");
return argcount;
} else if (!strcmp(arg, "--first-parent")) {
revs->first_parent_only = 1;
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 02/10] t5812: add 'test_i18ngrep's missing filename parameter
On Fri, Jan 26, 2018 at 01:37:00PM +0100, SZEDER Gábor wrote:
> [snip]
>
> diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
> index d911afd24..226a4920c 100755
> --- a/t/t5812-proto-disable-http.sh
> +++ b/t/t5812-proto-disable-http.sh
> @@ -21,8 +21,7 @@ test_expect_success 'curl redirects respect whitelist' '
> GIT_SMART_HTTP=0 \
> git clone "$HTTPD_URL/ftp-redir/repo.git" 2>stderr &&
> {
> - test_i18ngrep "ftp.*disabled" stderr ||
> - test_i18ngrep "your curl version is too old"
> + test_i18ngrep -E "(ftp.*disabled|your curl version is too old)"
> stderr
> }
I think we can drop the curly braces as well, as they were only
used to group the ||; leaving only:
> + test_i18ngrep -E "(ftp.*disabled|your curl version is too old)" stderr
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH v4 5/6] convert: add 'working-tree-encoding' attribute
On Mon, Jan 22, 2018 at 07:54:01PM -0500, Jeff King wrote: > But anyway, that was a bit of a tangent. Certainly the smaller change is > just standardizing on sizeof(*foo), which I think most people agree on > at this point. It might be worth putting in CodingGuidelines. Personally I prefer sizeof(*foo) which is a well non-idiom, used in many projects and IMHO easy to read and understand. I've played a little with coccinelle and the following spatch seems to catch many occurrences of sizeof(struct ..) (the first hunk seems to expand multiple times causing conflicts in the generated patch). Cases like a->b = xcalloc() are not matched, I don't know enough coccinelle for that. If there's interest I could prepare patches, but it will create quite some code churn. Regards Simon @@ type T; identifier x; @@ - T *x = xmalloc(sizeof(T)); + T *x = xmalloc(sizeof(*x)); @@ type T; T *x; @@ - x = xmalloc(sizeof(T)); + x = xmalloc(sizeof(*x)); @@ type T; identifier x; expression n; @@ - T *x = xcalloc(n, sizeof(T)); + T *x = xcalloc(n, sizeof(*x)); @@ type T; T *x; expression n; @@ - x = xcalloc(n, sizeof(T)); + x = xcalloc(n, sizeof(*x)); @@ type T; T x; @@ - memset(, 0, sizeof(T)); + memset(, 0, sizeof(x)); @@ type T; T *x; @@ - memset(x, 0, sizeof(T)); + memset(x, 0, sizeof(*x)); -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH v4 5/6] convert: add 'working-tree-encoding' attribute
On Mon, Jan 22, 2018 at 01:35:25PM +0100, Lars Schneider wrote: >>> + enc->name = xstrdup_toupper(value); /* aways use upper case names! */ >> >> "aways" -> "always" and I think the comment should say why >> uppercase is important. > > Would that be better? > > /* Aways use upper case names to simplify subsequent string comparison. > */ > enc->name = xstrdup_toupper(value); > > AFAIK uppercase and lowercase names are both valid. I just wanted to > ensure that we use one consistent casing. That reads better in error messages > and I don't need to check for the letter case in has_prohibited_utf_bom() > and friends in utf8.c Sounds good (minus the "Aways" typo Eric already noticed). >> Micro-nit: For consistency with the previous test, remove the >> empty line and comment (or just keep the files generated from the >> "setup test repo" phase and don't explicitly delete them)? > > I would rather add a new line and a comment to the previous test > to be consistent. > > I know we could leave the files but these lingering files could > always surprise writers of future tests (at least they surprised > me in other tests). Sure, that sounds good. Just noticed the inconsistency and wanted to mention it. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH v4 5/6] convert: add 'working-tree-encoding' attribute
On Sat, Jan 20, 2018 at 04:24:17PM +0100, lars.schnei...@autodesk.com wrote:
> +static struct encoding *git_path_check_encoding(struct attr_check_item
> *check)
> +{
> + const char *value = check->value;
> + struct encoding *enc;
> +
> + if (ATTR_TRUE(value) || ATTR_FALSE(value) || ATTR_UNSET(value) ||
> + !strlen(value))
> + return NULL;
> +
> + for (enc = encoding; enc; enc = enc->next)
> + if (!strcasecmp(value, enc->name))
> + return enc;
> +
> + /* Don't encode to the default encoding */
> + if (!strcasecmp(value, default_encoding))
> + return NULL;
> +
> + enc = xcalloc(1, sizeof(struct convert_driver));
I think this should be "sizeof(struct encoding)" but I prefer
"sizeof(*enc)" which prevents these kind of mistakes.
> + enc->name = xstrdup_toupper(value); /* aways use upper case names! */
"aways" -> "always" and I think the comment should say why
uppercase is important.
> +test_expect_success 'ensure UTF-8 is stored in Git' '
> + git cat-file -p :test.utf16 >test.utf16.git &&
> + test_cmp_bin test.utf8.raw test.utf16.git &&
> + rm test.utf8.raw test.utf16.git
> +'
> +
> +test_expect_success 're-encode to UTF-16 on checkout' '
> + rm test.utf16 &&
> + git checkout test.utf16 &&
> + test_cmp_bin test.utf16.raw test.utf16 &&
> +
> + # cleanup
> + rm test.utf16.raw
Micro-nit: For consistency with the previous test, remove the
empty line and comment (or just keep the files generated from the
"setup test repo" phase and don't explicitly delete them)?
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH/RFC] diff: add --compact-summary option to complement --stat
On Sat, Jan 13, 2018 at 08:22:11PM +0700, Nguyễn Thái Ngọc Duy wrote: > [snip] > > For mode changes, executable bit is denoted as "(+x)" or "(-x)" when > it's added or removed respectively. The same for when a regular file is > replaced with a symlink "(+l)" or the other way "(-l)". This also > applies to new files. New regulare files are "A", while new executable > files or symlinks are "A+x" or "A+l". I like the short summary. However I find the use of parentheses inconsistent. Why not use them either always (also for "(A+l)") or never? Was there a specific reason why you added them just in one place? Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
[PATCH] config: document default value of http.sslVerify
Remove any doubt that certificates might not be verified by default. Signed-off-by: Simon Ruderich <si...@ruderich.org> --- Documentation/config.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 3a1304874..0d49bcd70 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1968,8 +1968,8 @@ empty string. http.sslVerify:: Whether to verify the SSL certificate when fetching or pushing - over HTTPS. Can be overridden by the `GIT_SSL_NO_VERIFY` environment - variable. + over HTTPS. Defaults to true. Can be overridden by the + `GIT_SSL_NO_VERIFY` environment variable. http.sslCert:: File containing the SSL certificate when fetching or pushing -- 2.15.1 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 2/2] grep: fix segfault under -P + PCRE2 + (*NO_JIT)
On Wed, Nov 22, 2017 at 01:36:30PM +, Ævar Arnfjörð Bjarmason wrote:
> + *
> + * This is because if the pattern contains the
> + * (*NO_JIT) verb (see pcre2syntax(3))
> + * pcre2_jit_compile() will exit early with 0. If we
> + * then proceed to call pcre2_jit_match() further down
> + * the line instead of pcre2_match() we'll segfault.
> + */
> + patinforet = pcre2_pattern_info(p->pcre2_pattern,
> PCRE2_INFO_JITSIZE, );
> + if (patinforet)
> + die("BUG: The patinforet variable should be 0 after the
> pcre2_pattern_info() call, not %d",
> + patinforet);
I think BUG() should be used here, and maybe shorten the error
message:
BUG("pcre2_pattern_info() failed: %d", patinforet);
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())
On Mon, Nov 06, 2017 at 05:13:15PM +0100, Simon Ruderich wrote:
> On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote:
>> Yes, I think what you've written here (and below) is quite close to the
>> error_context patches I linked elsewhere in the thread. In other
>> words, I think it's a sane approach.
>
> In contrast to error_context I'd like to keep all exiting
> behavior (die, ignore, etc.) in the hand of the caller and not
> use any callbacks as that makes the control flow much harder to
> follow.
>
>> I agree it might be nice for the error context to have a positive "there
>> was an error" flag. It's probably worth making it redundant with the
>> return code, though, so callers can use whichever style is most
>> convenient for them.
>
> Agreed.
>
> Regarding the API, should it be allowed to pass NULL as error
> pointer to request no additional error handling or should the
> error functions panic on NULL? Allowing NULL makes partial
> conversions possible (e.g. for write_in_full) where old callers
> just pass NULL and check the return values and converted callers
> can use the error struct.
>
> How should translations get handled? Appending ": %s" for
> strerror(errno) might be problematic. Same goes for "outer
> message: inner message" where the helper function just inserts ":
> " between the messages. Is _("%s: %s") (with appropriate
> translator comments) enough to handle these cases?
>
> Suggestions how to name the struct and the corresponding
> functions? My initial idea was struct error and to use error_ as
> prefix, but I'm not sure if struct error is too broad and may
> introduce conflicts with system headers. Also error_ is a little
> long and could be shorted to just err_ but I don't know if that's
> clear enough. The error_ prefix doesn't conflict with many git
> functions, but there are some in usage.c (error_errno, error,
> error_routine).
>
> And as general question, is this approach to error handling
> something we should pursue or are there objections? If there's
> consensus that this might be a good idea I'll look into
> converting some parts of the git code (maybe refs.c) to see how
> it pans out.
Any comments?
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())
On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote:
> Yes, I think what you've written here (and below) is quite close to the
> error_context patches I linked elsewhere in the thread. In other
> words, I think it's a sane approach.
In contrast to error_context I'd like to keep all exiting
behavior (die, ignore, etc.) in the hand of the caller and not
use any callbacks as that makes the control flow much harder to
follow.
> I agree it might be nice for the error context to have a positive "there
> was an error" flag. It's probably worth making it redundant with the
> return code, though, so callers can use whichever style is most
> convenient for them.
Agreed.
Regarding the API, should it be allowed to pass NULL as error
pointer to request no additional error handling or should the
error functions panic on NULL? Allowing NULL makes partial
conversions possible (e.g. for write_in_full) where old callers
just pass NULL and check the return values and converted callers
can use the error struct.
How should translations get handled? Appending ": %s" for
strerror(errno) might be problematic. Same goes for "outer
message: inner message" where the helper function just inserts ":
" between the messages. Is _("%s: %s") (with appropriate
translator comments) enough to handle these cases?
Suggestions how to name the struct and the corresponding
functions? My initial idea was struct error and to use error_ as
prefix, but I'm not sure if struct error is too broad and may
introduce conflicts with system headers. Also error_ is a little
long and could be shorted to just err_ but I don't know if that's
clear enough. The error_ prefix doesn't conflict with many git
functions, but there are some in usage.c (error_errno, error,
error_routine).
And as general question, is this approach to error handling
something we should pursue or are there objections? If there's
consensus that this might be a good idea I'll look into
converting some parts of the git code (maybe refs.c) to see how
it pans out.
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 1/2] sequencer: factor out rewrite_file()
On Fri, Nov 03, 2017 at 03:13:10PM -0400, Jeff King wrote:
> I think we've been gravitating towards error strbufs, which would make
> it something like:
I like this approach to store the error in a separate variable
and let the caller handle it. This provides proper error messages
and is cleaner than printing the error on the error site (what
error_errno does).
However I wouldn't use strbuf directly and instead add a new
struct error which provides a small set of helper functions.
Using a separate type also makes it clear to the reader that is
not a normal string and is more extendable in the future.
> I'm not excited that the amount of error-handling code is now double the
> amount of code that actually does something useful. Maybe this function
> simply isn't large/complex enough to merit flexible error handling, and
> we should simply go with René's original near-duplicate.
A separate struct (and helper functions) would help in this case
and could look like this, which is almost equal (in code size) to
the original solution using error_errno:
int write_file_buf_gently2(const char *path, const char *buf, size_t len,
struct error *err)
{
int rc = 0;
int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
if (fd < 0)
return error_addf_errno(err, _("could not open '%s' for
writing"), path);
if (write_in_full(fd, buf, len) < 0)
rc = error_addf_errno(err, _("could not write to '%s'"),
path);
if (close(fd) && !rc)
rc = error_addf_errno(err, _("could not close '%s'"), path);
return rc;
}
(I didn't touch write_in_full here, but it could also take the
err and then the code would get a little shorter, however would
lose the "path" information, but see below.)
And in the caller:
void write_file_buf(const char *path, const char *buf, size_t len)
{
struct error err = ERROR_INIT;
if (write_file_buf_gently2(path, buf, len, ) < 0)
error_die();
}
For now struct error just contains the strbuf, but one could add
the call location (by using a macro for error_addf_errno) or the
original errno or more information in the future.
error_addf_errno() could also prepend the error the buffer so
that the caller can add more information if necessary and we get
something like: "failed to write file 'foo': write failed: errno
text" in the write_file_buf case (the first error string is from
write_file_buf_gently2, the second from write_in_full). However
I'm not sure how well this works with translations.
We could also store the error condition in the error struct and
don't use the return value to indicate and error like this:
void write_file_buf(const char *path, const char *buf, size_t len)
{
struct error err = ERROR_INIT;
write_file_buf_gently2(path, buf, len, );
if (err.error)
error_die();
}
> OTOH, if we went all-in on flexible error handling contexts, you could
> imagine this function becoming:
>
> void write_file_buf(const char *path, const char *buf, size_t len,
> struct error_context *err)
> {
> int fd = xopen(path, err, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> if (fd < 0)
> return -1;
> if (write_in_full(fd, buf, len, err) < 0)
> return -1;
> if (xclose(fd, err) < 0)
> return -1;
> return 0;
> }
This looks interesting as well, but it misses the feature of
custom error messages which is really useful.
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 1/2] sequencer: factor out rewrite_file()
On Wed, Nov 01, 2017 at 06:16:18PM -0400, Jeff King wrote:
> On Wed, Nov 01, 2017 at 10:46:14PM +0100, Johannes Schindelin wrote:
>> I spent substantial time on making the sequencer code libified (it was far
>> from it). That die() call may look okay now, but it is not at all okay if
>> we want to make Git's source code cleaner and more reusable. And I want
>> to.
>>
>> So my suggestion is to clean up write_file_buf() first, to stop behaving
>> like a drunk lemming, and to return an error value already, and only then
>> use it in sequencer.c.
>
> That would be fine with me, too.
I tried looking into this by adding a new write_file_buf_gently()
(or maybe renaming write_file_buf to write_file_buf_or_die) and
using it from write_file_buf() but I don't know the proper way to
handle the error-case in write_file_buf(). Just calling
die("write_file_buf") feels ugly, as the real error was already
printed on screen by error_errno() and I didn't find any function
to just exit without writing a message (which still respects
die_routine). Suggestions welcome.
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 1/2] wrapper.c: consistently quote filenames in error messages
On Thu, Nov 02, 2017 at 02:16:52PM +0900, Junio C Hamano wrote: > Junio C Hamano writes: >> This patch is incomplete without adjusting a handful of tests to >> expect the updated messages, no? > > I'll squash these in while queuing, but there might be more that I > didn't notice. Sorry, didn't think about the tests. I've re-checked and I think those are the only affected tests. The test suite passes with your squashed changes. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
[PATCH 2/2] sequencer.c: check return value of close() in rewrite_file()
Not checking close(2) can hide errors as not all errors are reported
during the write(2).
Signed-off-by: Simon Ruderich <si...@ruderich.org>
---
On Wed, Nov 01, 2017 at 02:00:11PM +0100, René Scharfe wrote:
> Most calls are not checked, but that doesn't necessarily mean they need
> to (or should) stay that way. The Linux man-page of close(2) spends
> multiple paragraphs recommending to check its return value.. Care to
> send a follow-up patch?
Hello,
Sure, here is it.
Regards
Simon
sequencer.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index f93b60f61..e0cc2f777 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2673,7 +2673,8 @@ static int rewrite_file(const char *path, const char
*buf, size_t len)
return error_errno(_("could not open '%s' for writing"), path);
if (write_in_full(fd, buf, len) < 0)
rc = error_errno(_("could not write to '%s'"), path);
- close(fd);
+ if (close(fd) && !rc)
+ rc = error_errno(_("could not close '%s'"), path);
return rc;
}
--
2.15.0
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
[PATCH 1/2] wrapper.c: consistently quote filenames in error messages
All other error messages in the file use quotes around the file name.
This change removes two translations as "could not write to '%s'" and
"could not close '%s'" are already translated and these two are the only
occurrences without quotes.
Signed-off-by: Simon Ruderich <si...@ruderich.org>
---
wrapper.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/wrapper.c b/wrapper.c
index 61aba0b5c..d20356a77 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -569,7 +569,7 @@ static int warn_if_unremovable(const char *op, const char
*file, int rc)
if (!rc || errno == ENOENT)
return 0;
err = errno;
- warning_errno("unable to %s %s", op, file);
+ warning_errno("unable to %s '%s'", op, file);
errno = err;
return rc;
}
@@ -583,7 +583,7 @@ int unlink_or_msg(const char *file, struct strbuf *err)
if (!rc || errno == ENOENT)
return 0;
- strbuf_addf(err, "unable to unlink %s: %s",
+ strbuf_addf(err, "unable to unlink '%s': %s",
file, strerror(errno));
return -1;
}
@@ -653,9 +653,9 @@ void write_file_buf(const char *path, const char *buf,
size_t len)
{
int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
if (write_in_full(fd, buf, len) < 0)
- die_errno(_("could not write to %s"), path);
+ die_errno(_("could not write to '%s'"), path);
if (close(fd))
- die_errno(_("could not close %s"), path);
+ die_errno(_("could not close '%s'"), path);
}
void write_file(const char *path, const char *fmt, ...)
--
2.15.0
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 1/2] sequencer: factor out rewrite_file()
On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:
> +static int rewrite_file(const char *path, const char *buf, size_t len)
> +{
> + int rc = 0;
> + int fd = open(path, O_WRONLY);
> + if (fd < 0)
> + return error_errno(_("could not open '%s' for writing"), path);
> + if (write_in_full(fd, buf, len) < 0)
> + rc = error_errno(_("could not write to '%s'"), path);
> + if (!rc && ftruncate(fd, len) < 0)
> + rc = error_errno(_("could not truncate '%s'"), path);
> + close(fd);
We might want to check the return value of close() as some file
systems report write errors only on close. But I'm not sure how
the rest of Git's code-base handles this.
> + return rc;
> +}
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 0/2] color-moved: ignore all space changes by default
On Wed, Oct 25, 2017 at 03:46:18PM -0700, Stefan Beller wrote: > On Mon, Oct 23, 2017 at 7:52 PM, Stefan Beller wrote[1]: >> On Mon, Oct 23, 2017 at 6:54 PM, Junio C Hamano wrote: >>> >>> * As moved-lines display is mostly a presentation thing, I wonder >>>if it makes sense to always match loosely wrt whitespace >>>differences. >> >> Well, sometimes the user wants to know if it is byte-for-byte identical >> (unlikely to be code, but maybe column oriented data for input; >> think of all our FORTRAN users. ;) > > ... and this is the implementation and the flip of the default setting > to ignore all white space for the move detection. Hello, I'm not sure if this is a good default. I think it's not obvious that moved code gets treated differently than regular changes. I wouldn't expect git diff to ignore whitespace changes (without me telling it to) and so when I see moved code I expect they were moved as is. And there are languages where indentation is relevant (e.g. Python, YAML) and as color-moved is also treated as review tool to detect unwanted changes this new default can be dangerous. The new options sound like a good addition but I don't think the defaults should change. However unrelated to this decision, please add config settings in addition to these new options so users can globally configure the behavior they want. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 2/3] t5615: avoid re-using descriptor 4
On Fri, Oct 20, 2017 at 06:46:08PM -0400, Jeff King wrote: >> I agree. Maybe just stick with the original patch? > > OK. Why don't we live with that for now, then. The only advantage of the > "999" trickery is that it's less likely to come up again. If it doesn't, > then we're happy. If it does, then we can always switch then. I think switching the 4 to 9 (which you already brought up in this thread) is a good idea. It makes accidental conflicts less likely (it's rare to use so many file descriptors) and is easy to implement. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 4/5] diff: fix whitespace-skipping with --color-moved
On Thu, Oct 19, 2017 at 04:29:26PM -0400, Jeff King wrote: > [snip] > > --- a/t/t4015-diff-whitespace.sh > +++ b/t/t4015-diff-whitespace.sh > @@ -1463,6 +1463,73 @@ test_expect_success 'move detection ignoring > whitespace changes' ' > test_cmp expected actual > ' > > +test_expect_failure 'move detection ignoring whitespace at eol' ' Shouldn't this be test_expect_success? According to the commit message "and a new "--ignore-space-at-eol" shows off the breakage we're fixing.". I didn't actually run the code so I don't know if the test fails or not. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 1/3] write_entry: fix leak when retrying delayed filter
On Tue, Oct 10, 2017 at 05:25:43AM -0400, Jeff King wrote:
> On Tue, Oct 10, 2017 at 11:23:19AM +0200, Simon Ruderich wrote:
>> On Tue, Oct 10, 2017 at 09:00:09AM +0900, Junio C Hamano wrote:
>>>> --- a/entry.c
>>>> +++ b/entry.c
>>>> @@ -283,6 +283,7 @@ static int write_entry(struct cache_entry *ce,
>>>>if (dco && dco->state != CE_NO_DELAY) {
>>>>/* Do not send the blob in case of a retry. */
>>>>if (dco->state == CE_RETRY) {
>>>> + free(new);
>>>>new = NULL;
>>>>size = 0;
>>>>}
>>
>> FREE_AND_NULL(new)?
>
> Ah, yeah, I forgot we had that now. It would work here, but note that
> this code ends up going away later in the series.
Ah sorry, missed that. Then never mind.
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 1/3] write_entry: fix leak when retrying delayed filter
On Tue, Oct 10, 2017 at 09:00:09AM +0900, Junio C Hamano wrote:
>> --- a/entry.c
>> +++ b/entry.c
>> @@ -283,6 +283,7 @@ static int write_entry(struct cache_entry *ce,
>> if (dco && dco->state != CE_NO_DELAY) {
>> /* Do not send the blob in case of a retry. */
>> if (dco->state == CE_RETRY) {
>> +free(new);
>> new = NULL;
>> size = 0;
>> }
FREE_AND_NULL(new)?
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH v3 03/10] protocol: introduce protocol extention mechanisms
On Tue, Oct 03, 2017 at 01:15:00PM -0700, Brandon Williams wrote: > [snip] > > +protocol.version:: > + Experimental. If set, clients will attempt to communicate with a > + server using the specified protocol version. If unset, no > + attempt will be made by the client to communicate using a > + particular protocol version, this results in protocol version 0 > + being used. > + Supported versions: Did you consider Stefan Beller's suggestion regarding a (white)list of allowed versions? On Mon, Sep 18, 2017 at 01:06:59PM -0700, Stefan Beller wrote: > Thinking about this, how about: > > If not configured, we do as we want. (i.e. Git has full control over > it's decision making process, which for now is "favor v0 over v1 as > we are experimenting with v1". This strategy may change in the future > to "prefer highest version number that both client and server can speak".) > > If it is configured, "use highest configured number from the given set". > > ? It would also allow the server operator to configure only a specific set of versions (to handle the "version x is insecure/slow"-issue raised by Stefan Beller). The current code always uses the latest protocol supported by the git binary. Minor nit, s/extention/extension/ in the patch name? Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: Are the 'How to' documents present as man pages?
On Sat, Sep 16, 2017 at 05:17:35PM +0530, Kaartic Sivaraam wrote: >> References to other man pages generally use round brackets, for >> example git-merge(1). > > I didn't know they had different meanings for different brackets in man > pages. [snip] Man pages in general use only round brackets to refer to another man page with the given section (like stat(1) or stat(2)). Square brackets have no special meaning, but are useful for references like URLs. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: Are the 'How to' documents present as man pages?
On Sat, Sep 16, 2017 at 10:30:43AM +0530, Kaartic Sivaraam wrote: > I was reading the 'git revert' documentation and found the following > line in it, > > -m parent-number > --mainline parent-number > > ... > > See the revert-a-faulty-merge How-To[1] for more details. Square brackets indicate links, you should have a NOTES section at the bottom of the man page which contains something like this: 1. revert-a-faulty-merge How-To file:///usr/share/doc/git/html/howto/revert-a-faulty-merge.html To my knowledge those are not available as man pages. > It says that the 'How-To' is present in the first section of the man > page. I tried to access it to get this, References to other man pages generally use round brackets, for example git-merge(1). I checked the git-scm.com website [1] and interestingly they use square brackets for these references which confused me a little. Not sure if it's worth changing though. Regards Simon [1]: https://git-scm.com/docs/git-revert -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH] hashmap: add API to disable item counting when threaded
On Wed, Aug 30, 2017 at 06:59:22PM +, Jeff Hostetler wrote:
> [snip]
> +/*
> + * Re-enable item couting when adding/removing items.
> + * If counting is currently disabled, it will force count them.
The code always recounts them. Either the comment or the
code should be adjusted.
> + * This might be used after leaving a threaded section.
> + */
> +static inline void hashmap_enable_item_counting(struct hashmap *map)
> +{
> + void *item;
> + unsigned int n = 0;
> + struct hashmap_iter iter;
> +
> + hashmap_iter_init(map, );
> + while ((item = hashmap_iter_next()))
> + n++;
> +
> + map->do_count_items = 1;
> + map->private_size = n;
> +}
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
git add -p breaks after split on change at the top of the file
Hello, The following session reproduces the issue with Git 2.14.1: $ git init test $ cd test $ echo x >file $ git add file $ git commit -m commit $ printf 'a\nb\nx\nc\n' >file $ git add -p diff --git a/file b/file index 587be6b..74a69a0 100644 --- a/file +++ b/file @@ -1 +1,4 @@ +a +b x +c Stage this hunk [y,n,q,a,d,/,s,e,?]? <-- press s Split into 2 hunks. @@ -1 +1,3 @@ +a +b x Stage this hunk [y,n,q,a,d,/,j,J,g,e,?]? <-- press e Now delete the line "+a" in your editor and save. error: patch failed: file:1 error: file: patch does not apply I expected git add -p to stage this change without error. It works fine without splitting the hunk (by deleting the first and last + line in the diff). Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH] strbuf: clear errno before calling getdelim(3)
On Fri, Aug 11, 2017 at 10:52:51AM +0200, René Scharfe wrote:
> Am 11.08.2017 um 09:50 schrieb Simon Ruderich:
>> On Thu, Aug 10, 2017 at 10:56:40PM +0200, René Scharfe wrote:
>>> getdelim(3) returns -1 at the end of the file and if it encounters an
>>> error, but sets errno only in the latter case. Set errno to zero before
>>> calling it to avoid misdiagnosing an out-of-memory condition due to a
>>> left-over value from some other function call.
>>
>> getdelim(3p) doesn't explicitly forbid changing the errno on EOF:
>>
>> If no characters were read, and the end-of-file indicator for
>> the stream is set, or if the stream is at end-of-file, the
>> end-of-file indicator for the stream shall be set and the
>> function shall return −1. If an error occurs, the error
>> indicator for the stream shall be set, and the function shall
>> return −1 and set errno to indicate the error.
>
> [snip]
>
>> I don't think that it matters in practice, but the "most" correct
>> way to handle this would be to check if feof(3) is true to check
>> for the non-errno case.
>
> Only if errors at EOF are guaranteed to be impossible. Imagine
> getdelim being able to read to the end and then failing to get memory
> for the final NUL. Other scenarios may be possible.
Good point. Instead of feof(3), checking ferror(3) should handle
that properly. It's guaranteed to be set by getdelim for any
error.
For example like this (as replacement for the original patch):
diff --git i/strbuf.c w/strbuf.c
index 89d22e3b0..831be21ce 100644
--- i/strbuf.c
+++ w/strbuf.c
@@ -495,7 +495,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int
term)
* memory and retry, but that's unlikely to help for a malloc small
* enough to hold a single line of input, anyway.
*/
- if (errno == ENOMEM)
+ if (ferror(fp) && errno == ENOMEM)
die("Out of memory, getdelim failed");
/*
An edge case is if the error indicator was already set before
calling getdelim() and the errno was already set to ENOMEM. This
could be fixed by checking ferror() before calling getdelim, but
I'm not sure if that's necessary:
diff --git i/strbuf.c w/strbuf.c
index 89d22e3b0..4d394bb91 100644
--- i/strbuf.c
+++ w/strbuf.c
@@ -468,7 +468,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int
term)
{
ssize_t r;
- if (feof(fp))
+ if (feof(fp) || ferror(fp))
return EOF;
strbuf_reset(sb);
Regards
Simon
--
+ Privatsphäre ist notwendig
+ Ich verwende GnuPG http://gnupg.org
+ Öffentlicher Schlüssel: 0x92FEFDB7E44C32F9
Re: [PATCH] strbuf: clear errno before calling getdelim(3)
On Thu, Aug 10, 2017 at 10:56:40PM +0200, René Scharfe wrote: > getdelim(3) returns -1 at the end of the file and if it encounters an > error, but sets errno only in the latter case. Set errno to zero before > calling it to avoid misdiagnosing an out-of-memory condition due to a > left-over value from some other function call. getdelim(3p) doesn't explicitly forbid changing the errno on EOF: If no characters were read, and the end-of-file indicator for the stream is set, or if the stream is at end-of-file, the end-of-file indicator for the stream shall be set and the function shall return −1. If an error occurs, the error indicator for the stream shall be set, and the function shall return −1 and set errno to indicate the error. So a valid implementation could still set errno on EOF and also on another error (where it's required to set errno). I don't think that it matters in practice, but the "most" correct way to handle this would be to check if feof(3) is true to check for the non-errno case. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 25/25] diff: document the new --color-moved setting
On Fri, Jun 30, 2017 at 09:04:50AM -0700, Stefan Beller wrote: > [snip] > > However > > context > + moved line, block A or B > + moved line, block A or B > context > > is omitted, because the number of lines > here is fewer than 3 ignoring the block > type. > > Maybe > > If there are fewer than 3 adjacent lines of > moved code, they are skipped. My issue with "skipped" is that it's not clear what exactly is skipped. Move detection? Inclusion in the diff? Something else? That's why I tried to add the "excluded from move detection" to make it clear that the change is still shown to the user, just not handled by move detection and using the usual diff colors. In your example above, what exactly is "omitted"? The complete hunk from the output or the special coloring? That's what confuses me and what might confuse a future reader of the documentation. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH 25/25] diff: document the new --color-moved setting
On Thu, Jun 29, 2017 at 05:07:10PM -0700, Stefan Beller wrote: > + Small blocks of 3 moved lines or fewer are skipped. If I read the commit messages correctly, this "skipping" process applies to the move detection in general for those smaller blocks and therefore doesn't mean a malicious move can hide smaller changes, correct? If so, I find this sentence misleading. Maybe something like: Small blocks of 3 moved lines or fewer are excluded from move detection and colored as regular diff. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: your mail
On Thu, Jun 22, 2017 at 01:55:27PM +, Patrick Lehmann wrote: > The description on https://github.com/git/git doesn't reflect that policy. > > a) > It explains that discussions take place in the mentioned mailing list. > b) > It describes how to subscribe. However it doesn't say that you have to subscribe to send, only how to subscribe. > With knowledge of other mailing lists (mostly managed by mailman), > subscription is required for participation. That depends on the mailing list, some require subscription to prevent spams but not all do. Somebody might want to update the documentation, but personally I see no reason to change anything. If you want to send a patch to improve it, that would be great of course. Regards Simon PS: Please don't top-post on this mailing list. -- + Privatsphäre ist notwendig + Ich verwende GnuPG http://gnupg.org + Öffentlicher Schlüssel: 0x92FEFDB7E44C32F9
Re: your mail
On Thu, Jun 22, 2017 at 01:35:33PM +, Patrick Lehmann wrote: > But how can he write to the mailing list without a subscription? > Is this a security bug or is he already subscribed? Everybody can send to this mailing list. This is by design so contributors/bug reporters can send mails without having to subscribe. Regards Simon -- + Privatsphäre ist notwendig + Ich verwende GnuPG http://gnupg.org + Öffentlicher Schlüssel: 0x92FEFDB7E44C32F9
No "invalid option" message with git diff --cached --invalid-option
Hello, I'm using Git 2.13.1 (from the Debian sid repository) and noticed the following issue when upgrading. $ git diff --compaction-heuristic error: invalid option: --compaction-heuristic $ git diff --cached --compaction-heuristic usage: git diff [] [ []] [--] [...] I know that --compaction-heuristic is no longer supported but I was using it an alias and was confused that I got no proper error message warning me which option was wrong. It seems to happen for any invalid option which is used in combination with --cached or --staged. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: your mail
On Thu, Jun 22, 2017 at 11:50:01AM +0200, Jessie Hernandez wrote: > subscribe git You need to write to majord...@vger.kernel.org (with subscribe git in the body) to subscribe. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH] die routine: change recursion limit from 1 to 1024
On Tue, Jun 20, 2017 at 08:49:59PM +0200, Ævar Arnfjörð Bjarmason wrote:
> If I understand you correctly this on top:
>
> diff --git a/usage.c b/usage.c
> index 1c198d4882..f6d5af2bb4 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -46,7 +46,19 @@ static int die_is_recursing_builtin(void)
> static int dying;
> static int recursion_limit = 1024;
>
> - return dying++ > recursion_limit;
> + dying++;
> +
> + if (!dying) {
This will never trigger as dying was incremented two lines
before. But I think it's already handled by the dying <
recursion_limit case so we can just omit it.
> + /* ok, normal */
> + return 0;
> + } else if (dying < recursion_limit) {
> + /* only show the warning once */
> + if (dying == 1)
> + warning("die() called many times. Recursion error or
> racy threaded death!");
> + return 0; /* don't bail yet */
> + } else {
> + return 1;
> + }
> }
Maybe restructure it like this:
dying++
if (dying > recursion_limit)
return 1;
if (dying == 1)
warning();
return 0;
Btw. is there a reason why recursion_limit is a static variable
and not a constant/define?
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH v2 1/2] refs: Add for_each_worktree_ref for iterating over all worktree HEADs
On Wed, May 17, 2017 at 06:45:31PM -0700, Manish Goregaokar wrote: > Hm, my invocation of git-send-email keeps getting the threading wrong. > Is there a recommended set of arguments to the command? The threading looks fine here (for both cases where you mentioned it being wrong). Why do you think it's wrong? How does it look on your end? Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
Re: [PATCH v2 4/7] grep: add support for the PCRE v1 JIT API
On Sat, May 13, 2017 at 11:45:32PM +, Ævar Arnfjörð Bjarmason wrote:
> [snip]
>
> +#ifdef PCRE_CONFIG_JIT
> + if (p->pcre1_jit_on)
> + ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
> + eol - line, 0, flags, ovector,
> + ARRAY_SIZE(ovector), p->pcre1_jit_stack);
> + else
> + ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
> + eol - line, 0, flags, ovector,
> + ARRAY_SIZE(ovector));
> +#else
> ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
> 0, flags, ovector, ARRAY_SIZE(ovector));
> +#endif
Wouldn't it be simpler to remove the duplication and
unconditionally use the old pcre_exec() call? Something like
this:
+#ifdef PCRE_CONFIG_JIT
+ if (p->pcre1_jit_on)
+ ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
+ eol - line, 0, flags, ovector,
+ ARRAY_SIZE(ovector), p->pcre1_jit_stack);
+ else
+#endif
ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
0, flags, ovector, ARRAY_SIZE(ovector));
> if (ret < 0 && ret != PCRE_ERROR_NOMATCH)
> die("pcre_exec failed with error code %d", ret);
> if (ret > 0) {
> @@ -394,7 +420,16 @@ static int pcre1match(struct grep_pat *p, const char
> *line, const char *eol,
> static void free_pcre1_regexp(struct grep_pat *p)
> {
> pcre_free(p->pcre1_regexp);
> +#ifdef PCRE_CONFIG_JIT
> + if (p->pcre1_jit_on) {
> + pcre_free_study(p->pcre1_extra_info);
> + pcre_jit_stack_free(p->pcre1_jit_stack);
> + } else {
> + pcre_free(p->pcre1_extra_info);
> + }
> +#else
> pcre_free(p->pcre1_extra_info);
> +#endif
Same here. The pcre_free() is the same with and without the
ifdef.
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: [PATCH v2] l10n: de.po: update German translation
On Sun, Apr 30, 2017 at 09:11:49PM +0200, Ralf Thielow wrote: > #: config.c:1952 > #, c-format > msgid "unknown core.untrackedCache value '%s'; using 'keep' default value" > -msgstr "" > +msgstr "Unbekannter Wert '%s' in core.untrackedCache; benutze Stardardwert > 'keep'" Standardwert > #: entry.c:280 > -#, fuzzy, c-format > +#, c-format > msgid "could not stat file '%s'" > -msgstr "konnte Datei '%s' nicht erstellen" > +msgstr "konnte Datei '%s' nicht lesen" Read is not quite stat (there are situations where you can stat but not read a file), but I can't think of a better translation. > #: builtin/describe.c:551 > -#, fuzzy > msgid "--broken is incompatible with commit-ishes" > -msgstr "Die Option --dirty kann nicht mit Commits verwendet werden." > +msgstr "Die Option --broken kann nicht nit Commits verwendet werden." ^^^ mit > #: builtin/tag.c:312 > msgid "tag: tagging " > -msgstr "" > +msgstr "Tag: tagge " Lowercase Tag because it's used as command prefix? In other places in this patch the lowercase version was used for commands. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
[PATCH] githooks.txt: clarify push hooks are always executed in $GIT_DIR
Listing the specific hooks might feel verbose but without it the
reader is left to wonder which hooks are triggered during the
push. Something which is not immediately obvious when only trying
to find out where the hook is executed.
Signed-off-by: Simon Ruderich <si...@ruderich.org>
---
Documentation/githooks.txt | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
On Mon, Apr 10, 2017 at 01:13:15PM +0200, Ævar Arnfjörð Bjarmason wrote:
> [snip]
>
> Can we say as we do now that:
>
> * All hooks regardless of type in bare repos execute in the bare repo
> * If you have a working tree hooks use that
>
> But add:
>
> * Working trees are ignored by any hooks invoked on your behalf during a push.
Hello,
Maybe like this? I reordered the cases as it felt more natural
that the general case is first and followed by the one with the
exception.
> Some ad-hoc testing reveals that this rule also goes for the
> push-to-checkout hook. Should it? Wouldn't it be more useful if it
> broke the pattern, since it's dealing with the working tree on the
> other side? Junio?
I added push-to-checkout to the patch. Changing the behavior will
break backwards compatibility so I think that's a no-go.
Regards
Simon
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 32343ae29..706091a56 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -22,8 +22,10 @@ changed via the `core.hooksPath` configuration variable (see
linkgit:git-config[1]).
Before Git invokes a hook, it changes its working directory to either
-the root of the working tree in a non-bare repository, or to the
-$GIT_DIR in a bare repository.
+$GIT_DIR in a bare repository or the root of the working tree in a non-bare
+repository. An exception are hooks triggered during a push ('pre-receive',
+'update', 'post-receive', 'post-update', 'push-to-checkout') which are always
+executed in $GIT_DIR.
Hooks can get their arguments via the environment, command-line
arguments, and stdin. See the documentation for each hook below for
--
2.11.0
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Unexpected working directory in post-receive hook in non-bare repository
Hello, The following snippet reproduces the issue for me (note the remote: line in its output): git --version rm -rf a b git init a cd a echo first >data git add data git commit -m initial cat >>.git/hooks/post-receive <>data git add data git commit -m test git push origin master:not-master According to man githooks "Before Git invokes a hook, it changes its working directory to either the root of the working tree in a non-bare repository, [...]". In this case "a" is non-bare and I expected the command to be run in the working tree; but instead it's run inside .git. (This caused some confusion in my case because I ran "git merge" in the hook which put files in the .git directory and I didn't notice it at first. I know running merge in receive-hooks is "bad practice" but it works fine in my setup.) The same happens for all hooks executed by git-receive-pack: pre-receive, update, post-receive, post-update. Is this a documentation issue or unexpected behavior? Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 signature.asc Description: PGP signature
Re: [PATCH] push: document & test --force-with-lease with multiple remotes
Hello, I like the documentation update and test. On Sat, Apr 08, 2017 at 11:41:00AM +, Ævar Arnfjörð Bjarmason wrote: > [snip] > > ++ > +Now when the background process runs `git fetch origin` the references > +on `origin-push` won't be updated, and thus commands like: > ++ > + git push --force-with-lease origin I think this should be origin-push. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 signature.asc Description: PGP signature
Re: [PATCH 2/4] t7610: make tests more independent and debuggable
On Tue, Jan 03, 2017 at 07:50:40PM -0500, Richard Hansen wrote: > [snip] > @@ -145,8 +148,13 @@ test_expect_success 'mergetool in subdir' ' > ' > > test_expect_success 'mergetool on file in parent dir' ' > + git reset --hard && > + git checkout -b test$test_count branch1 && > + git submodule update -N && > ( > cd subdir && > + test_must_fail git merge master >/dev/null 2>&1 && > + ( yes "" | git mergetool file3 >/dev/null 2>&1 ) && This change seems unrelated to the changes mentioned in the commit message. Was it intended? Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 signature.asc Description: PGP signature
Re: [ANNOUNCE] Git v2.11.0-rc0
On Mon, Oct 31, 2016 at 02:49:42PM -0700, Junio C Hamano wrote: > [snip] Hello, I noticed a few minor typos in the changelog. > * The default abbreviation length, which has historically been 7, now >scales as the repository grows, using the approximate number of >objects in the reopsitory and a bit of math around the birthday ^^ repository >paradox. The logic suggests to use 12 hexdigits for the Linux >kernel, and 9 to 10 for Git itself. > > > Updates since v2.10 > --- > > [snip] > > * "git clone --resurse-submodules --reference $path $URL" is a way to ^^^ recurse >reduce network transfer cost by borrowing objects in an existing >$path repository when cloning the superproject from $URL; it >learned to also peek into $path for presense of corresponding presence > [snip] > > * Output from "git diff" can be made easier to read by selecting >which lines are common and which lines are added/deleted >intelligently when the lines before and after the changed section >are the same. A command line option is added to help with the >experiment to find a good heuristics. Maybe the name of the command line option should be added here. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 signature.asc Description: PGP signature
Re: [PATCH 27/36] attr: convert to new threadsafe API
On Wed, Oct 26, 2016 at 10:52:23AM +0200, Johannes Schindelin wrote:
> diff --git a/attr.c b/attr.c
> index d5a6aa9..6933504 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -50,7 +50,16 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);
> #ifndef NO_PTHREADS
>
> static pthread_mutex_t attr_mutex;
> -#define attr_lock()pthread_mutex_lock(_mutex)
> +static inline void attr_lock(void)
> +{
> + static int initialized;
> +
> + if (!initialized) {
> + pthread_mutex_init(_mutex, NULL);
> + initialized = 1;
> + }
> + pthread_mutex_lock(_mutex);
> +}
This may initialize the mutex multiple times during the first
lock (which may happen in parallel).
pthread provides static initializers. To quote the man page:
Variables of type pthread_mutex_t can also be initialized
statically, using the constants PTHREAD_MUTEX_INITIALIZER
(for fast mutexes), PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
(for recursive mutexes), and
PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP (for error checking
mutexes).
Regards
Simon
--
+ Privatsphäre ist notwendig
+ Ich verwende GnuPG http://gnupg.org
+ Öffentlicher Schlüssel: 0x92FEFDB7E44C32F9
signature.asc
Description: PGP signature
Crash when using git blame on untracked file
Hello,
I'm seeing the following crash with Git 2.9.3 on Debian sid
(amd64):
$ git init foo
$ cd foo
$ touch x
$ git add x
$ git commit -m test
$ touch x.conf
$ git blame x.conf
segmentation fault
I've tested it on Debian stable's 2.1.4 which works fine:
$ git blame x.conf
fatal: no such path 'x.conf' in HEAD
It requires the blamed file to be present, but in some cases it
works only if the file e.g. "x" is already tracked in Git and the
other file is called "x.conf" (".conf"-suffix). But in an empty
repo it seems to happen always.
Sadly Debian's git has no dbg-package, so the stacktrace is not
very useful:
#0 __strcmp_sse2_unaligned () at
../sysdeps/x86_64/multiarch/strcmp-sse2-unaligned.S:31
#1 0x0041ad7a in ?? ()
#2 0x00406171 in ?? ()
#3 0x00405321 in ?? ()
#4 0x76f9f700 in __libc_start_main (main=0x4051c0, argc=0x3,
argv=0x7fffe1a8, init=, fini=,
rtld_fini=, stack_end=0x7fffe198)
at ../csu/libc-start.c:291
#5 0x004057d9 in ?? ()
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
signature.asc
Description: PGP signature
Re: Repository formats
On Tue, Apr 01, 2014 at 10:18:55AM -0400, Phillip Susi wrote: I have seen some discussion about various changes to the format of the index and pack files over time, but can't find anything about it in the man pages. Are the different formats documented anywhere, and how to tell which format you are using? Hello, The documentation is available in Documentation/technical/, e.g. index-format.txt and pack-format.txt. However not everything is available there. For current work on those formats, check the mailing list archive, e.g. [1] Regards Simon [1]: http://permalink.gmane.org/gmane.comp.version-control.git/233083 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: File extension conflict when working with git and latex
On Fri, Mar 21, 2014 at 05:13:24PM +0100, Matthias Beyer wrote: Unfortunetely, we wrote our `make clean` task recursively. I think you can imagine what went wrong: The clean-task corrupted the repository, as it removed .idx files from within .git/. I lost work because of this ugly name collision. Hello Matthias, You can recreate the .idx files by running git index-pack .git/objects/pack/pack-hash.pack for each pack file. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: File extension conflict when working with git and latex
On Fri, Mar 21, 2014 at 05:46:51PM +0100, Matthias Beyer wrote: Hi Simon, I think so. I executed: git fsck # reports N missing blobs, commits, trees and dangling stuff git index-pack ... git fsck # reports only dangling commits and blobs I don't know if this means that the repository is fixed now? Hello Matthias, If nothing else was deleted, then yes, the repository should be fine now. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clean: respect pathspecs with -d
On Mon, Mar 10, 2014 at 01:22:15PM -0400, Jeff King wrote: +test_expect_success 'git clean -d respects pathspecs' ' + mkdir foo + mkdir foobar + git clean -df foobar + test_path_is_dir foo + test_path_is_missing foobar +' + test_done I think we should also test removing foo, which was also in the original report, to make sure we don't match prefixes, e.g.: test_expect_success 'git clean -d respects pathspecs' ' mkdir foo mkdir foobar git clean -df foo test_path_is_missing foo test_path_is_dir foobar ' Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git-config: document interactive.singlekey requires Term::ReadKey
Most distributions don't require Term::ReadKey as dependency, leaving
the user to wonder why the setting doesn't work.
Signed-off-by: Simon Ruderich si...@ruderich.org
---
On Mon, Mar 03, 2014 at 10:58:58AM -0800, Junio C Hamano wrote:
Thanks, but is it true that interactive.singlekey requries
Term::ReadKey?
Yes, it requires it. The code also works fine without
Term::ReadKey, but the feature singlekey requires this module.
I assumed a user enabling this option would also want to use the
feature, therefore requires is fine IMHO.
The implementation of prompt_single_character sub wants to use
ReadKey, but can still let the user interact with the program by
falling back to a cooked input when it is not available, so perhaps
a better fix might be something like this:
if (!$use_readkey) {
print STDERR missing Term::ReadKey, disabling
interactive.singlekey\n;
}
inside the above if() that prepares $use_readkey?
Good idea. Implemented in an additional patch.
I think the documentation should also be updated (this patch) to
make it clear to a reader of the man page, that an additional
module is required, without having him to try to use the option.
You also misspelled the package name it seems ;-)
Oops, sorry. Fixed in this reroll.
Regards
Simon
Documentation/config.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5f4d793..406a582 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1633,7 +1633,7 @@ interactive.singlekey::
linkgit:git-add[1], linkgit:git-checkout[1], linkgit:git-commit[1],
linkgit:git-reset[1], and linkgit:git-stash[1]. Note that this
setting is silently ignored if portable keystroke input
- is not available.
+ is not available; requires the Perl module Term::ReadKey.
log.abbrevCommit::
If true, makes linkgit:git-log[1], linkgit:git-show[1], and
--
1.9.0.11.g9a08b42
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-add--interactive: warn if module for interactive.singlekey is missing
Suggested-by: Junio C Hamano gits...@pobox.com
Signed-off-by: Simon Ruderich si...@ruderich.org
---
git-add--interactive.perl | 3 +++
1 file changed, 3 insertions(+)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 24bb1ab..d3bca12 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -58,6 +58,9 @@ if ($repo-config_bool(interactive.singlekey)) {
Term::ReadKey-import;
$use_readkey = 1;
};
+ if (!$use_readkey) {
+ print STDERR missing Term::ReadKey, disabling
interactive.singlekey\n;
+ }
eval {
require Term::Cap;
my $termcap = Term::Cap-Tgetent;
--
1.9.0.11.g9a08b42
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-config: document interactive.singlekey requires Term::Readkey
Most distributions don't require Term::Readkey as dependency, leaving the user to wonder why the setting doesn't work. Signed-off-by: Simon Ruderich si...@ruderich.org --- Documentation/config.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 5f4d793..ec26fa8 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1633,7 +1633,7 @@ interactive.singlekey:: linkgit:git-add[1], linkgit:git-checkout[1], linkgit:git-commit[1], linkgit:git-reset[1], and linkgit:git-stash[1]. Note that this setting is silently ignored if portable keystroke input - is not available. + is not available; requires the Perl module Term::Readkey. log.abbrevCommit:: If true, makes linkgit:git-log[1], linkgit:git-show[1], and -- 1.9.0.11.g9a08b42 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: `git stash pop` UX Problem
On Mon, Feb 24, 2014 at 05:21:40PM +0100, Matthieu Moy wrote: One easy thing to do OTOH would be to show a hint at the end of git stash pop's output, like I think that's a good idea. It makes it obvious that Git has kept the stash and that the user should drop it when he's done - if he wants to. $ git stash pop Auto-merging foo.txt CONFLICT (content): Merge conflict in foo.txt 'stash pop' failed. Please, resolve the conflicts manually. The stash was not dropped in case you need to restart the operation. When you are done resolving the merge, you may run the following to drop the stash: git stash drop Maybe just the following to keep the output on a single line: Use 'git stash drop' to remove the stash after resolving the conflicts. But maybe that's too short as it doesn't mention explicitly, that the stash was kept. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Git v1.8.4.2 test failure in ./t5570-git-daemon.sh
Hello, I just compiled Git v1.8.4.2 on Debian Wheezy amd64 and test t5570 fails (with GIT_TEST_GIT_DAEMON=1): --- expect 2013-10-28 23:27:26.792409631 + +++ output 2013-10-28 23:27:26.788409614 + @@ -1 +1,2 @@ +Cloning into 'nowhere'... fatal: remote error: access denied or repository not exported: /nowhere.git [18908] [19625] Disconnected (with error) not ok 9 - clone non-existent --- expect 2013-10-28 23:27:26.944410377 + +++ output 2013-10-28 23:27:26.944410377 + @@ -1 +1,2 @@ +Cloning into 'nowhere'... fatal: remote error: no such repository: /nowhere.git [19727] [19747] Disconnected (with error) not ok 13 - clone non-existent Bisecting leads to this commit: commit 68b939b2f097b6675c4aaa17869aa81b25cb Author: Jeff King p...@peff.net Date: Wed Sep 18 16:05:13 2013 -0400 clone: send diagnostic messages to stderr Putting messages like Cloning into.. and done on stdout is un-Unix and uselessly clutters the stdout channel. Send them to stderr. We have to tweak two tests to accommodate this: 1. t5601 checks for doubled output due to forking, and doesn't actually care where the output goes; adjust it to check stderr. 2. t5702 is trying to test whether progress output was sent to stderr, but naively does so by checking whether stderr produced any output. Instead, have it look for %, a token found in progress output but not elsewhere (and which lets us avoid hard-coding the progress text in the test). This should not regress any scripts that try to parse the current output, as the output is already internationalized and therefore unstable. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gitweb: Fix the author initials in blame for non-ASCII names
On Fri, Aug 30, 2013 at 11:13:19AM -0700, Junio C Hamano wrote: I think in this function the filehandle is called $fd, not $fh. Has any of you really tested this??? I did, but I applied the change by hand without applying the patch directly and didn't notice the difference. Sorry for that. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: The gitweb author initials feature from a36817b doesn't work with i18n names
On Thu, Aug 29, 2013 at 04:26:29PM +0200, Ævar Arnfjörð Bjarmason wrote: I haven't gotten an env where I can test gitweb running, but that looks like it should work to me. I've tested the patch and it works fine. Tested-by: Simon Ruderich si...@ruderich.org Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log -p unexpected behaviour - security risk?
On Thu, Apr 11, 2013 at 11:36:26AM +0100, John Tapsell wrote:
Is there a way to make --cc default?
If you use aliases, something like this is easy:
git config --global --add alias.lp 'log --patch --cc'
I use aliases heavily, so that's my fix for now.
But I think the current behaviour is unexpected for most (new?)
users (including me). I thought -p would display all changes in
all commits, including merges.
I guess changing -p's default behaviour to imply --cc is
problematic, so I think we should document that -p doesn't
generate patches for merges. Maybe something like this:
-- 8 --
Subject: [PATCH] Documentation/diff-options.txt: -p doesn't display merge
changes
Signed-off-by: Simon Ruderich si...@ruderich.org
---
Documentation/diff-options.txt | 4
1 file changed, 4 insertions(+)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 104579d..cd35ec7 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -24,6 +24,10 @@ ifndef::git-format-patch[]
--patch::
Generate patch (see section on generating patches).
{git-diff? This is the default.}
+ifdef::git-log[]
+ Changes introduced in merge commits are not displayed. Use `-c`,
+ `--cc` or `-m` to include them.
+endif::git-log[]
endif::git-format-patch[]
-Un::
--
1.8.2.1.513.gdedbb69.dirty
-- 8 --
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-imap-send.txt: remove the use of sslverify=false in GMail example
On Thu, Apr 11, 2013 at 06:55:03PM +0300, Barbu Paul - Gheorghe wrote: Should I create a new patch removing them all? Sounds like a good idea to me. And update the commit message with Junio's suggestions. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] help: mark common_guides[] as translatable
Signed-off-by: Simon Ruderich si...@ruderich.org
---
On Tue, Apr 02, 2013 at 11:39:51PM +0100, Philip Oakley wrote:
--- a/help.c
+++ b/help.c
@@ -240,6 +241,23 @@ void list_common_cmds_help(void)
}
}
+void list_common_guides_help(void)
+{
+ int i, longest = 0;
+
+ for (i = 0; i ARRAY_SIZE(common_guides); i++) {
+ if (longest strlen(common_guides[i].name))
+ longest = strlen(common_guides[i].name);
+ }
+
+ puts(_(The common Git guides are:\n));
+ for (i = 0; i ARRAY_SIZE(common_guides); i++) {
+ printf( %s , common_guides[i].name);
+ mput_char(' ', longest - strlen(common_guides[i].name));
+ puts(_(common_guides[i].help));
common_guides[] is used here, but without N_() not picked up by
xgettext when creating the pot file.
Regards
Simon
builtin/help.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/builtin/help.c b/builtin/help.c
index 034c36c..062957f 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -419,13 +419,13 @@ static struct {
const char *name;
const char *help;
} common_guides[] = {
- { attributes, Defining attributes per path },
- { glossary, A Git glossary },
- { ignore, Specifies intentionally untracked files to ignore },
- { modules, Defining submodule properties },
- { revisions, Specifying revisions and ranges for Git },
- { tutorial, A tutorial introduction to Git (for version 1.5.1 or
newer) },
- { workflows, An overview of recommended workflows with Git},
+ { attributes, N_(Defining attributes per path) },
+ { glossary, N_(A Git glossary) },
+ { ignore, N_(Specifies intentionally untracked files to ignore) },
+ { modules, N_(Defining submodule properties) },
+ { revisions, N_(Specifying revisions and ranges for Git) },
+ { tutorial, N_(A tutorial introduction to Git (for version 1.5.1 or
newer)) },
+ { workflows, N_(An overview of recommended workflows with Git) },
};
static void list_common_guides_help(void)
--
1.8.2.481.g0d034d4
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-imap-send.txt: remove the use of sslverify=false in GMail example
On Wed, Apr 10, 2013 at 11:44:03AM -0700, Junio C Hamano wrote: The reason why we can run with sslverify=true against gmail is because we know imap.gmail.com gives a validly signed certificate that leads all the way to a root CA the user's OpenSSL installation is likely to trust (if your hand-rolled imap-over-ssl server uses a snakeoil certificate, even though the server may be SSL capable, you may not be able to successfully connect to it without sslverify turned off). Maybe imap-send should learn imap.sslCAInfo and imap.sslCAPath like http.* to handle custom certificates. diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt index 875d283..b15dffe 100644 --- a/Documentation/git-imap-send.txt +++ b/Documentation/git-imap-send.txt @@ -123,7 +123,6 @@ to specify your account settings: host = imaps://imap.gmail.com user = u...@gmail.com port = 993 -sslverify = false - You might need to instead use: folder = [Google Mail]/Drafts if you get an error I think we should remove sslverify = false from the other example as well. Recommending sslverify = false is IMHO a bad idea as SSL provides no protection without verification. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] t/README: --immediate skips cleanup commands for failed tests
On Sun, Apr 07, 2013 at 03:32:00PM -0700, Jonathan Nieder wrote: I'm not sure if it's better to use test_when_finished with rm or just rm -rf tmp at the end of the test in case someone wants to look at the output. test_when_finished is better here, since it means later tests can run and provide useful information about how bad a regression is. Cleanup commands requested using test_when_finished are not run when a test being run with --immediate fails, so you can still inspect output after a failed test. Hello Jonathan, Thanks for the explanation. I couldn't find this documented in t/README, the following patch adds it. -- 8 -- Subject: [PATCH] t/README: --immediate skips cleanup commands for failed tests --- t/README | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/README b/t/README index 9b41fe7..e5e7d37 100644 --- a/t/README +++ b/t/README @@ -86,7 +86,8 @@ appropriately before running make. --immediate:: This causes the test to immediately exit upon the first - failed test. + failed test. Cleanup commands requested with + test_when_finished are not executed if the test failed. --long-tests:: This causes additional long-running tests to be run (where -- 1.8.2.481.g0d034d4 -- 8 -- Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] t/README: --immediate skips cleanup commands for failed tests
Signed-off-by: Simon Ruderich si...@ruderich.org --- On Tue, Apr 09, 2013 at 12:16:56PM -0700, Junio C Hamano wrote: Sign-off? Sorry, forgot it. Perhaps adding ... to keep the state for inspection by the tester to diagnose the bug or something is in order? Good idea. Revised patch is attached. Regards Simon t/README | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/t/README b/t/README index 9b41fe7..e6c060e 100644 --- a/t/README +++ b/t/README @@ -86,7 +86,10 @@ appropriately before running make. --immediate:: This causes the test to immediately exit upon the first - failed test. + failed test. Cleanup commands requested with + test_when_finished are not executed if the test failed to + keep the state for inspection by the tester to diagnose + the bug. --long-tests:: This causes additional long-running tests to be run (where -- 1.8.2.481.g0d034d4 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/6] format-patch: add format.cover-letter configuration
On Sun, Apr 07, 2013 at 12:46:23PM -0500, Felipe Contreras wrote: [snip] +test_expect_success 'cover letter auto' ' + mkdir -p tmp + test_when_finished rm -rf tmp; + git config --unset format.coverletter + + git config format.coverletter auto test_config format.coverletter auto takes automatically care of the git config --unset cleanup. I'm not sure if it's better to use test_when_finished with rm or just rm -rf tmp at the end of the test in case someone wants to look at the output. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/3] diffcore-pickaxe: respect --no-textconv
git log -S doesn't respect --no-textconv:
$ echo '*.txt diff=wrong' .gitattributes
$ git -c diff.wrong.textconv='xxx' log --no-textconv -Sfoo
error: cannot run xxx: No such file or directory
fatal: unable to read files to diff
Reported-by: Matthieu Moy matthieu@grenoble-inp.fr
Signed-off-by: Simon Ruderich si...@ruderich.org
---
On Fri, Apr 05, 2013 at 09:40:21AM +0200, Matthieu Moy wrote:
While we're there, we could test -G --no-textconv too.
Hello Matthieu,
Good idea, I've added it.
Regards
Simon
diffcore-pickaxe.c | 12
t/t4209-log-pickaxe.sh | 28
2 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 3124f49..26ddf00 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -86,8 +86,10 @@ static int diff_grep(struct diff_filepair *p, struct
diff_options *o,
if (diff_unmodified_pair(p))
return 0;
- textconv_one = get_textconv(p-one);
- textconv_two = get_textconv(p-two);
+ if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
+ textconv_one = get_textconv(p-one);
+ textconv_two = get_textconv(p-two);
+ }
mf1.size = fill_textconv(textconv_one, p-one, mf1.ptr);
mf2.size = fill_textconv(textconv_two, p-two, mf2.ptr);
@@ -201,8 +203,10 @@ static int has_changes(struct diff_filepair *p, struct
diff_options *o,
if (!o-pickaxe[0])
return 0;
- textconv_one = get_textconv(p-one);
- textconv_two = get_textconv(p-two);
+ if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
+ textconv_one = get_textconv(p-one);
+ textconv_two = get_textconv(p-two);
+ }
/*
* If we have an unmodified pair, we know that the count will be the
diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index eed7273..38fb80f 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -80,6 +80,20 @@ test_expect_success 'log -G -i (match)' '
test_cmp expect actual
'
+test_expect_success 'log -G --textconv (missing textconv tool)' '
+ echo * diff=test .gitattributes
+ test_must_fail git -c diff.test.textconv=missing log -Gfoo
+ rm .gitattributes
+'
+
+test_expect_success 'log -G --no-textconv (missing textconv tool)' '
+ echo * diff=test .gitattributes
+ git -c diff.test.textconv=missing log -Gfoo --no-textconv actual
+ expect
+ test_cmp expect actual
+ rm .gitattributes
+'
+
test_expect_success 'log -S (nomatch)' '
git log -Spicked --format=%H actual
expect
@@ -116,4 +130,18 @@ test_expect_success 'log -S -i (nomatch)' '
test_cmp expect actual
'
+test_expect_success 'log -S --textconv (missing textconv tool)' '
+ echo * diff=test .gitattributes
+ test_must_fail git -c diff.test.textconv=missing log -Sfoo
+ rm .gitattributes
+'
+
+test_expect_success 'log -S --no-textconv (missing textconv tool)' '
+ echo * diff=test .gitattributes
+ git -c diff.test.textconv=missing log -Sfoo --no-textconv actual
+ expect
+ test_cmp expect actual
+ rm .gitattributes
+'
+
test_done
--
1.8.2
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv()
On Thu, Apr 04, 2013 at 01:48:52PM -0700, Junio C Hamano wrote: If I am reading the code correctly, it is has_changes(), which is used for log -S (not log -G that uses diff_grep()), that does the unnecessary get_textconv() unconditionally. The way diff_grep() divides the work to make fill_one() responsible for filling the textconv as necessary is internally consistent, and there is no unnecessary call. Yes, of course. I meant has_changes() which has the unnecessary call. Perhaps... The fill_one() function is responsible for finding and filling the textconv filter as necessary, and is called by diff_grep() function that implements git log -Gpattern. The has_changes() function calls get_textconv() for two sides being compared, before it checks to see if it was asked to perform the pickaxe limiting with the -S option. Move the code around to avoid this wastage. After that, fill_one() is called to use the textconv. By adding get_textconv() to diff_grep() and relieving fill_one() of responsibility to find the textconv filter, we can avoid calling get_textconv() twice. Sounds good to me. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] diffcore-pickaxe: respect --no-textconv
git log -S doesn't respect --no-textconv:
$ echo '*.txt diff=wrong' .gitattributes
$ git -c diff.wrong.textconv='xxx' log --no-textconv -Sfoo
error: cannot run xxx: No such file or directory
fatal: unable to read files to diff
Signed-off-by: Simon Ruderich si...@ruderich.org
Reported-by: Matthieu Moy matthieu@grenoble-inp.fr
---
On Thu, Apr 04, 2013 at 10:34:17AM +0200, Matthieu Moy wrote:
Hi,
It seems the command git log --no-textconv -Sfoo still runs the
textconv filter (noticed because I have a broken textconv filter that
lets git log -S error out).
[snip]
Hello Matthieu,
This patch should fix it. All tests pass.
Regards
Simon
diffcore-pickaxe.c | 15 ---
t/t4209-log-pickaxe.sh | 14 ++
2 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index b097fa7..f814a52 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -78,7 +78,6 @@ static void fill_one(struct diff_filespec *one,
mmfile_t *mf, struct userdiff_driver **textconv)
{
if (DIFF_FILE_VALID(one)) {
- *textconv = get_textconv(one);
mf-size = fill_textconv(*textconv, one, mf-ptr);
} else {
memset(mf, 0, sizeof(*mf));
@@ -97,6 +96,11 @@ static int diff_grep(struct diff_filepair *p, struct
diff_options *o,
if (diff_unmodified_pair(p))
return 0;
+ if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
+ textconv_one = get_textconv(p-one);
+ textconv_two = get_textconv(p-two);
+ }
+
fill_one(p-one, mf1, textconv_one);
fill_one(p-two, mf2, textconv_two);
@@ -201,14 +205,19 @@ static unsigned int contains(mmfile_t *mf, struct
diff_options *o,
static int has_changes(struct diff_filepair *p, struct diff_options *o,
regex_t *regexp, kwset_t kws)
{
- struct userdiff_driver *textconv_one = get_textconv(p-one);
- struct userdiff_driver *textconv_two = get_textconv(p-two);
+ struct userdiff_driver *textconv_one = NULL;
+ struct userdiff_driver *textconv_two = NULL;
mmfile_t mf1, mf2;
int ret;
if (!o-pickaxe[0])
return 0;
+ if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
+ textconv_one = get_textconv(p-one);
+ textconv_two = get_textconv(p-two);
+ }
+
/*
* If we have an unmodified pair, we know that the count will be the
* same and don't even have to load the blobs. Unless textconv is in
diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index eed7273..953cec8 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -116,4 +116,18 @@ test_expect_success 'log -S -i (nomatch)' '
test_cmp expect actual
'
+test_expect_success 'log -S --textconv (missing textconv tool)' '
+ echo * diff=test .gitattributes
+ test_must_fail git -c diff.test.textconv=missing log -Sfoo
+ rm .gitattributes
+'
+
+test_expect_success 'log -S --no-textconv (missing textconv tool)' '
+ echo * diff=test .gitattributes
+ git -c diff.test.textconv=missing log -Sfoo --no-textconv actual
+ expect
+ test_cmp expect actual
+ rm .gitattributes
+'
+
test_done
--
1.8.2
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] diffcore-pickaxe: fill_one() doesn't modify textconv
Signed-off-by: Simon Ruderich si...@ruderich.org
---
diffcore-pickaxe.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index f814a52..c378ea0 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -75,10 +75,10 @@ static void diffgrep_consume(void *priv, char *line,
unsigned long len)
}
static void fill_one(struct diff_filespec *one,
-mmfile_t *mf, struct userdiff_driver **textconv)
+mmfile_t *mf, struct userdiff_driver *textconv)
{
if (DIFF_FILE_VALID(one)) {
- mf-size = fill_textconv(*textconv, one, mf-ptr);
+ mf-size = fill_textconv(textconv, one, mf-ptr);
} else {
memset(mf, 0, sizeof(*mf));
}
@@ -101,8 +101,8 @@ static int diff_grep(struct diff_filepair *p, struct
diff_options *o,
textconv_two = get_textconv(p-two);
}
- fill_one(p-one, mf1, textconv_one);
- fill_one(p-two, mf2, textconv_two);
+ fill_one(p-one, mf1, textconv_one);
+ fill_one(p-two, mf2, textconv_two);
if (!mf1.ptr) {
if (!mf2.ptr)
@@ -228,8 +228,8 @@ static int has_changes(struct diff_filepair *p, struct
diff_options *o,
if (textconv_one == textconv_two diff_unmodified_pair(p))
return 0;
- fill_one(p-one, mf1, textconv_one);
- fill_one(p-two, mf2, textconv_two);
+ fill_one(p-one, mf1, textconv_one);
+ fill_one(p-two, mf2, textconv_two);
if (!mf1.ptr) {
if (!mf2.ptr)
--
1.8.2
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv()
get_textconv() is called in diff_grep() to determine the textconv driver
before calling fill_one() and then again in fill_one(). Remove this
unnecessary call by determining the textconv driver before calling
fill_one().
With this change it's also no longer necessary for fill_one() to
modify the textconv argument, therefore pass a pointer instead of
a pointer to a pointer.
Signed-off-by: Simon Ruderich si...@ruderich.org
---
Hello,
I've reordered the patches as requested and included Jeff's
cleanup patch.
Regards
Simon
diffcore-pickaxe.c | 23 ++-
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index b097fa7..8f955f8 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -75,11 +75,10 @@ static void diffgrep_consume(void *priv, char *line,
unsigned long len)
}
static void fill_one(struct diff_filespec *one,
-mmfile_t *mf, struct userdiff_driver **textconv)
+mmfile_t *mf, struct userdiff_driver *textconv)
{
if (DIFF_FILE_VALID(one)) {
- *textconv = get_textconv(one);
- mf-size = fill_textconv(*textconv, one, mf-ptr);
+ mf-size = fill_textconv(textconv, one, mf-ptr);
} else {
memset(mf, 0, sizeof(*mf));
}
@@ -97,8 +96,11 @@ static int diff_grep(struct diff_filepair *p, struct
diff_options *o,
if (diff_unmodified_pair(p))
return 0;
- fill_one(p-one, mf1, textconv_one);
- fill_one(p-two, mf2, textconv_two);
+ textconv_one = get_textconv(p-one);
+ textconv_two = get_textconv(p-two);
+
+ fill_one(p-one, mf1, textconv_one);
+ fill_one(p-two, mf2, textconv_two);
if (!mf1.ptr) {
if (!mf2.ptr)
@@ -201,14 +203,17 @@ static unsigned int contains(mmfile_t *mf, struct
diff_options *o,
static int has_changes(struct diff_filepair *p, struct diff_options *o,
regex_t *regexp, kwset_t kws)
{
- struct userdiff_driver *textconv_one = get_textconv(p-one);
- struct userdiff_driver *textconv_two = get_textconv(p-two);
+ struct userdiff_driver *textconv_one = NULL;
+ struct userdiff_driver *textconv_two = NULL;
mmfile_t mf1, mf2;
int ret;
if (!o-pickaxe[0])
return 0;
+ textconv_one = get_textconv(p-one);
+ textconv_two = get_textconv(p-two);
+
/*
* If we have an unmodified pair, we know that the count will be the
* same and don't even have to load the blobs. Unless textconv is in
@@ -219,8 +224,8 @@ static int has_changes(struct diff_filepair *p, struct
diff_options *o,
if (textconv_one == textconv_two diff_unmodified_pair(p))
return 0;
- fill_one(p-one, mf1, textconv_one);
- fill_one(p-two, mf2, textconv_two);
+ fill_one(p-one, mf1, textconv_one);
+ fill_one(p-two, mf2, textconv_two);
if (!mf1.ptr) {
if (!mf2.ptr)
--
1.8.2
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] diffcore-pickaxe: remove fill_one()
From: Jeff King p...@peff.net
fill_one is _almost_ identical to just calling fill_textconv; the
exception is that for the !DIFF_FILE_VALID case, fill_textconv gives us
an empty buffer rather than a NULL one.
Signed-off-by: Simon Ruderich si...@ruderich.org
---
On Thu, Apr 04, 2013 at 01:49:42PM -0400, Jeff King wrote:
[snip]
What do you think of something like this on top (this is on top of your
patches, but again, I would suggest re-ordering your two, so it would
come as patch 2/3):
Hello Jeff,
That's a good idea. I've added your patch, thanks. Signed-off?
Regards
Simon
diffcore-pickaxe.c | 30 ++
1 file changed, 10 insertions(+), 20 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 8f955f8..3124f49 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -74,16 +74,6 @@ static void diffgrep_consume(void *priv, char *line,
unsigned long len)
line[len] = hold;
}
-static void fill_one(struct diff_filespec *one,
-mmfile_t *mf, struct userdiff_driver *textconv)
-{
- if (DIFF_FILE_VALID(one)) {
- mf-size = fill_textconv(textconv, one, mf-ptr);
- } else {
- memset(mf, 0, sizeof(*mf));
- }
-}
-
static int diff_grep(struct diff_filepair *p, struct diff_options *o,
regex_t *regexp, kwset_t kws)
{
@@ -99,15 +89,15 @@ static int diff_grep(struct diff_filepair *p, struct
diff_options *o,
textconv_one = get_textconv(p-one);
textconv_two = get_textconv(p-two);
- fill_one(p-one, mf1, textconv_one);
- fill_one(p-two, mf2, textconv_two);
+ mf1.size = fill_textconv(textconv_one, p-one, mf1.ptr);
+ mf2.size = fill_textconv(textconv_two, p-two, mf2.ptr);
- if (!mf1.ptr) {
- if (!mf2.ptr)
+ if (!DIFF_FILE_VALID(p-one)) {
+ if (!DIFF_FILE_VALID(p-two))
return 0; /* ignore unmerged */
/* created two -- does it have what we are looking for? */
hit = !regexec(regexp, mf2.ptr, 1, regmatch, 0);
- } else if (!mf2.ptr) {
+ } else if (!DIFF_FILE_VALID(p-two)) {
/* removed one -- did it have what we are looking for? */
hit = !regexec(regexp, mf1.ptr, 1, regmatch, 0);
} else {
@@ -224,16 +214,16 @@ static int has_changes(struct diff_filepair *p, struct
diff_options *o,
if (textconv_one == textconv_two diff_unmodified_pair(p))
return 0;
- fill_one(p-one, mf1, textconv_one);
- fill_one(p-two, mf2, textconv_two);
+ mf1.size = fill_textconv(textconv_one, p-one, mf1.ptr);
+ mf2.size = fill_textconv(textconv_two, p-two, mf2.ptr);
- if (!mf1.ptr) {
- if (!mf2.ptr)
+ if (!DIFF_FILE_VALID(p-one)) {
+ if (!DIFF_FILE_VALID(p-two))
ret = 0; /* ignore unmerged */
/* created */
ret = contains(mf2, o, regexp, kws) != 0;
}
- else if (!mf2.ptr) /* removed */
+ else if (!DIFF_FILE_VALID(p-two)) /* removed */
ret = contains(mf1, o, regexp, kws) != 0;
else
ret = contains(mf1, o, regexp, kws) !=
--
1.8.2
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/3] diffcore-pickaxe: respect --no-textconv
git log -S doesn't respect --no-textconv:
$ echo '*.txt diff=wrong' .gitattributes
$ git -c diff.wrong.textconv='xxx' log --no-textconv -Sfoo
error: cannot run xxx: No such file or directory
fatal: unable to read files to diff
Reported-by: Matthieu Moy matthieu@grenoble-inp.fr
Signed-off-by: Simon Ruderich si...@ruderich.org
---
diffcore-pickaxe.c | 12
t/t4209-log-pickaxe.sh | 14 ++
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 3124f49..26ddf00 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -86,8 +86,10 @@ static int diff_grep(struct diff_filepair *p, struct
diff_options *o,
if (diff_unmodified_pair(p))
return 0;
- textconv_one = get_textconv(p-one);
- textconv_two = get_textconv(p-two);
+ if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
+ textconv_one = get_textconv(p-one);
+ textconv_two = get_textconv(p-two);
+ }
mf1.size = fill_textconv(textconv_one, p-one, mf1.ptr);
mf2.size = fill_textconv(textconv_two, p-two, mf2.ptr);
@@ -201,8 +203,10 @@ static int has_changes(struct diff_filepair *p, struct
diff_options *o,
if (!o-pickaxe[0])
return 0;
- textconv_one = get_textconv(p-one);
- textconv_two = get_textconv(p-two);
+ if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
+ textconv_one = get_textconv(p-one);
+ textconv_two = get_textconv(p-two);
+ }
/*
* If we have an unmodified pair, we know that the count will be the
diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index eed7273..953cec8 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -116,4 +116,18 @@ test_expect_success 'log -S -i (nomatch)' '
test_cmp expect actual
'
+test_expect_success 'log -S --textconv (missing textconv tool)' '
+ echo * diff=test .gitattributes
+ test_must_fail git -c diff.test.textconv=missing log -Sfoo
+ rm .gitattributes
+'
+
+test_expect_success 'log -S --no-textconv (missing textconv tool)' '
+ echo * diff=test .gitattributes
+ git -c diff.test.textconv=missing log -Sfoo --no-textconv actual
+ expect
+ test_cmp expect actual
+ rm .gitattributes
+'
+
test_done
--
1.8.2
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] contrib/subtree: remove use of -a/-o in [ commands
From: Paul Campbell pcampb...@kemitix.net
Use of -a and -o in the [ command can have confusing semantics.
Use a separate test invocation for each single test, combining them with
and ||.
Signed-off-by: Paul Campbell pcampb...@kemitix.net
Signed-off-by: Simon Ruderich si...@ruderich.org
---
On Sun, Mar 24, 2013 at 07:37:42PM +, Paul Campbell wrote:
Use a separate test invocation for each single test, combining them with
and ||, and use ordinary parentheses for grouping.
Hello Paul,
Parentheses are only necessary if both and || are used to
enforce precedence; the shell can split the commands without
needing the parentheses. In these cases they can all be removed.
Regards
Simon
contrib/subtree/git-subtree.sh | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 5701376..d02e6c5 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -119,7 +119,7 @@ esac
dir=$(dirname $prefix/.)
-if [ $command != pull -a $command != add -a $command != push ];
then
+if test $command != pull test $command != add test $command !=
push; then
revs=$(git rev-parse $default --revs-only $@) || exit $?
dirs=$(git rev-parse --no-revs --no-flags $@) || exit $?
if [ -n $dirs ]; then
@@ -181,9 +181,9 @@ cache_set()
{
oldrev=$1
newrev=$2
- if [ $oldrev != latest_old \
--a $oldrev != latest_new \
--a -e $cachedir/$oldrev ]; then
+ if test $oldrev != latest_old \
+ test $oldrev != latest_new \
+ test -e $cachedir/$oldrev; then
die cache for $oldrev already exists!
fi
echo $newrev $cachedir/$oldrev
@@ -273,12 +273,12 @@ find_existing_splits()
git-subtree-split:) sub=$b ;;
END)
debug Main is: '$main'
- if [ -z $main -a -n $sub ]; then
+ if test -z $main test -n $sub; then
# squash commits refer to a subtree
debug Squash: $sq from $sub
cache_set $sq $sub
fi
- if [ -n $main -a -n $sub ]; then
+ if test -n $main test -n $sub; then
debug Prior: $main - $sub
cache_set $main $sub
cache_set $sub $sub
@@ -541,7 +541,7 @@ cmd_add_commit()
tree=$(git write-tree) || exit $?
headrev=$(git rev-parse HEAD) || exit $?
- if [ -n $headrev -a $headrev != $rev ]; then
+ if test -n $headrev test $headrev != $rev; then
headp=-p $headrev
else
headp=
--
1.8.2
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-am: fix Applying message when applypatch-hook was run
--- Hello, This patch fixes a minor issue with git-am. When the applypatch-hook modifies the commit message, git-am displays the original message. This patch updates the message to use the modified version. Regards Simon git-am.sh | 8 1 file changed, 8 insertions(+) diff --git a/git-am.sh b/git-am.sh index 202130f..0997077 100755 --- a/git-am.sh +++ b/git-am.sh @@ -795,6 +795,14 @@ To restore the original branch and stop patching run \\$cmdline --abort\. then $GIT_DIR/hooks/applypatch-msg $dotest/final-commit || stop_here $this + + # applypatch-msg can update the commit message. + if test -f $dotest/final-commit + then + FIRSTLINE=$(sed 1q $dotest/final-commit) + else + FIRSTLINE= + fi fi say $(eval_gettext Applying: \$FIRSTLINE) -- 1.8.2 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-am: fix Applying message when applypatch-hook was run
applypatch-hook can modify the commit message. Display the updated commit message instead of the original one. Signed-off-by: Simon Ruderich si...@ruderich.org --- On Thu, Mar 21, 2013 at 12:36:00AM +0100, Matthieu Moy wrote: Please, read SubmittingPatches in the Documentation directory of Git's source tree. Your text above should be a commit message (hence, no hello), and should not be below the --- line. Also, read about signed-off-by in the same document. Hello Matthieu, Thank you for the suggestions. I've adapted the patch message and added the signed-off. This copy/paste a piece of code that is already a few lines above. Is there any reason not to _move_ the assignment to FIRSTLINE after the if test -x $GIT_DIR/hooks/applypatch-msg, to avoid duplicating? No, there wasn't a reason not to move the code. I just wasn't sure if it had any side effects. But I rechecked and it should work fine. Updating version attached. On Wed, Mar 20, 2013 at 04:52:43PM -0700, Junio C Hamano wrote: More importantly, is this change even desirable? The original motivation behind the Applying: message was to help the user identify which one of the 100+ patches being fed to the command, and it was not about showing what we ended up committing. When you are running the command interactively, we do grab the edited result since f23272f3fd84 (git-am -i: report rewritten title, 2007-12-04), but I tend to feel that the automated munging done by applypatch-msg falls into a different category. When I first used the applypatch-msg hook I was confused because the messages were different and I thought the hook wasn't working, hence the patch. I'm not sure how extensive most applypatch-msg hooks modify the commit message (in my case just a number prepended), but I think it's more natural and less confusing to see the message which is being applied. If the original behaviour is preferred, a short comment in githooks(5) should prevent any confusion. Regards Simon git-am.sh | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/git-am.sh b/git-am.sh index 202130f..c092855 100755 --- a/git-am.sh +++ b/git-am.sh @@ -778,13 +778,6 @@ To restore the original branch and stop patching run \\$cmdline --abort\. action=yes fi - if test -f $dotest/final-commit - then - FIRSTLINE=$(sed 1q $dotest/final-commit) - else - FIRSTLINE= - fi - if test $action = skip then go_next @@ -797,6 +790,13 @@ To restore the original branch and stop patching run \\$cmdline --abort\. stop_here $this fi + if test -f $dotest/final-commit + then + FIRSTLINE=$(sed 1q $dotest/final-commit) + else + FIRSTLINE= + fi + say $(eval_gettext Applying: \$FIRSTLINE) case $resolved in -- 1.8.2 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
