Re: Get "responsible" .gitignore file / rule
On Fri, Dec 7, 2018 at 7:36 AM Victor Toni wrote: > I'm wondering if there is any way to show which rules (ideally with > the .gitignore file they are coming from) are causing a specific file > to get ignored so I could easily fix the .gitignore file? Perhaps the "git check-ignore" command would help.
Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files
On Tue, Dec 4, 2018 at 11:28 AM Duy Nguyen wrote: > Haven't really worked on killing the term "detached HEAD" yet. But I > noticed the other day that git-branch reports > > * (HEAD detached from 703266f6e4) > > and I didn't know how to rephrase that. I guess "unnamed branch from > 703266f6e4" is probably good enough but my old-timer brain screams no. "git worktree add" and "git worktree show" also report similar messages.
[PATCH v3] range-diff: always pass at least minimal diff options
From: Martin Ågren
Commit d8981c3f88 ("format-patch: do not let its diff-options affect
--range-diff", 2018-11-30) taught `show_range_diff()` to accept a
NULL-pointer as an indication that it should use its own "reasonable
default". That fixed a regression from a5170794 ("Merge branch
'ab/range-diff-no-patch'", 2018-11-18), but unfortunately it introduced
a regression of its own.
In particular, it means we forget the `file` member of the diff options,
so rather than placing a range-diff in the cover-letter, we write it to
stdout. In order to fix this, rewrite the two callers adjusted by
d8981c3f88 to instead create a "dummy" set of diff options where they
only fill in the fields we absolutely require, such as output file and
color.
Modify and extend the existing tests to try and verify that the right
contents end up in the right place.
Don't revert `show_range_diff()`, i.e., let it keep accepting NULL.
Rather than removing what is dead code and figuring out it isn't
actually dead and we've broken 2.20, just leave it for now.
[es: retain diff coloring when going to stdout]
Signed-off-by: Martin Ågren
Signed-off-by: Eric Sunshine
---
This is a re-roll of Martin's v2[1]. The only difference from v2 is that
it retains coloring when emitting to the terminal (plus an in-code
comment was simplified).
The regression introduced by d8981c3f88, in which the range-diff only
ever gets emitted to the terminal, and never to the cover letter or
commentary section of a standalone patch, makes the --range-diff option
rather useless, so this fix probably ought to be fast-tracked. An
alternative would be to rip out all the recent "--range-diff"-related
changes and go with the --range-diff implementation which has been in
use for a few months, even if it is not perfect.
[1]:
https://public-inbox.org/git/20181203200734.527341-1-martin.ag...@gmail.com/
builtin/log.c | 11 ++-
log-tree.c| 11 ++-
t/t3206-range-diff.sh | 20 +---
3 files changed, 33 insertions(+), 9 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 5ac18e2848..e8e51068bd 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1094,9 +1094,18 @@ static void make_cover_letter(struct rev_info *rev, int
use_stdout,
}
if (rev->rdiff1) {
+ /*
+* Pass minimum required diff-options to range-diff; others
+* can be added later if deemed desirable.
+*/
+ struct diff_options opts;
+ diff_setup();
+ opts.file = rev->diffopt.file;
+ opts.use_color = rev->diffopt.use_color;
+ diff_setup_done();
fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
show_range_diff(rev->rdiff1, rev->rdiff2,
- rev->creation_factor, 1, NULL);
+ rev->creation_factor, 1, );
}
}
diff --git a/log-tree.c b/log-tree.c
index b243779a0b..10680c139e 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -755,14 +755,23 @@ void show_log(struct rev_info *opt)
if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
struct diff_queue_struct dq;
+ struct diff_options opts;
memcpy(, _queued_diff, sizeof(diff_queued_diff));
DIFF_QUEUE_CLEAR(_queued_diff);
next_commentary_block(opt, NULL);
fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
+ /*
+* Pass minimum required diff-options to range-diff; others
+* can be added later if deemed desirable.
+*/
+ diff_setup();
+ opts.file = opt->diffopt.file;
+ opts.use_color = opt->diffopt.use_color;
+ diff_setup_done();
show_range_diff(opt->rdiff1, opt->rdiff2,
- opt->creation_factor, 1, NULL);
+ opt->creation_factor, 1, );
memcpy(_queued_diff, , sizeof(diff_queued_diff));
}
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index e497c1358f..048feaf6dd 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -248,18 +248,24 @@ test_expect_success 'dual-coloring' '
for prev in topic master..topic
do
test_expect_success "format-patch --range-diff=$prev" '
- git format-patch --stdout --cover-letter --range-diff=$prev \
+ git format-patch --cover-letter --range-diff=$prev \
master..unmodified >actual &&
- grep "= 1: .* s/5/A" actual &&
- grep "= 2: .* s/4/A" actual &&
- grep "= 3: .* s/11/B" actual &&
- grep &q
Re: [PATCH] rebase -i: introduce the 'test' command
On Sat, Dec 1, 2018 at 3:02 PM Jeff King wrote: > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: > > In reality, I think that it would even make sense to change the default to > > reschedule failed `exec` commands. Which is why I suggested to also add a > > config option. > > I sometimes add "x false" to the top of the todo list to stop and create > new commits before the first one. That would be awkward if I could never > get past that line. However, I think elsewhere a "pause" line has been > discussed, which would serve the same purpose. Are you thinking of the "break" command (not "pause") which Dscho already added[1]? [1]: 71f82465b1 (rebase -i: introduce the 'break' command, 2018-10-12)
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
On Thu, Nov 29, 2018 at 11:03 AM Ævar Arnfjörð Bjarmason wrote: > I mean not just nasty in terms of implementation, yeah we could do it, > but also a nasty UX for things like --word-diff-regex. I.e. instead of: > > --range-diff-word-diff-regex='[0-9"]' > > You need: > > --range-diff-opts="--word-diff-regex='[0-9\"]'" > > Now admittedly that in itself isn't very painful *in this case*, but in > terms of precedent I really dislike that option, i.e. git having some > mode where I need to work to escape input to pass to another command. > > Not saying that this --range-diff-* thing is what we should go for, but > surely we can find some way to do deal with this that doesn't involve > the user needing to escape stuff like this. I should mention that it was never the intention that git-format-patch's --range-diff option would allow passing _all_ possible options to git-range-diff, but only those most likely to be tweaked by the user (such as --creation-factor). It was understood from the start (and stated by me either in the cover letter to that series or in discussion) that the user always has the escape hatch of running git-range-diff manually and copy/pasting its output into the cover letter. So, I'm not convinced that we need this sort of flexibility in git-format-patch's --range-diff option, but we certainly can add convenience options (such as I did with --creation-factor) as people complain that their favorite option is missing. For a UI, I think we already have sufficient precedent for "--range-diff-opts=nopatch,creation-factor:60" as a possibility.
Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)
On Thu, Nov 29, 2018 at 11:27 PM Junio C Hamano wrote: > Junio C Hamano writes: > > In any case, I tend to agree with the conclusion in the downthread > > by Dscho that we should just clearly mark that invocations of the > > "format-patch --range-diff" command with additional diff options is > > an experimental feature that may not do anything sensible in the > > upcoming release, and declare that the UI to pass diff options to > > affect only the range-diff part may later be invented. IOW, I am > > coming a bit stronger than Dscho's suggestion in that we should not > > even pretend that we aimed to make the options used for range-diff > > customizable when driven from format-patch in the upcoming release, > > or aimed to make --range-diff option compatible with other diff > > options given to the format-patch command. I agree that this way forward makes sense. It's clear that I overlooked how there could be unexpected interactions from passing git-format-patch's own diff_options to show_range_diff(), so taking time to think it through without the pressure of a looming release is preferable to rushing out some "fixes". > So how about doing this on top of 'master' instead? As this leaks > *no* information wrt how range-diff machinery should behave from the > format-patch side by not passing any diffopt, as long as the new > code I added to show_range_diff() comes up with a reasonable default > diffopts (for which I really would appreciate extra sets of eyes to > make sure), this change by definition cannot be wrong (famous last > words). I, myself, was going to suggest this approach of leaking none of the git-format-patch's options into range_diff(), so I think it is a good one. Later, we can selectively pass certain _sensible_ options into show_range_diff() once we figure out the correct UI (for instance, --range-diff-opts=nopatch,creation-factor:60). A couple comments on the patch itself... > diff --git a/range-diff.c b/range-diff.c > @@ -460,7 +460,11 @@ int show_range_diff(const char *range1, const char > *range2, > - memcpy(, diffopt, sizeof(opts)); > + if (diffopt) > + memcpy(, diffopt, sizeof(opts)); > + else > + repo_diff_setup(the_repository, ); The first attempt at adding --range-diff to git-format-patch invoked the git-range-diff command, so no diff_options were passed at all. After Dscho libified the range-diff machinery in one of his major re-rolls, I took advantage of that to avoid the subprocess invocation. Another benefit of calling show_range_diff() directly is that when "git format-patch --stdout --range-diff=..." is sent to the terminal, the range-diff gets colored output for free. I'm pleased to see that that still works after this change. > diff --git a/range-diff.h b/range-diff.h > @@ -5,6 +5,11 @@ > +/* > + * Compare series of commmits in RANGE1 and RANGE2, and emit to the > + * standard output. NULL can be passed to DIFFOPT to use the built-in > + * default. > + */ It is more correct to say that the range-diff is emitted to diffopt->file (which may be stdout). Thanks for working on this.
Re: [bug report] git-gui child windows are blank
On Wed, Nov 28, 2018 at 2:29 PM Stefan Beller wrote: > On Wed, Nov 28, 2018 at 6:13 AM Kenn Sebesta wrote: > > v2.19.2, installed from brew on macOS Mojave 14.2.1. > > > > `git-gui` is my much beloved go-to tool for everything git. > > Unfortunately, on my new Macbook Air it seems to have a bug. When I > > first load the program, the parent window populates normally with the > > stage/unstaged and diff panes. However, when I click Push, the child > > window is completely blank. The frame is there, but there is no > > content. > > Looking through the mailing list archive, this > seemed to be one of the last relevant messges > https://public-inbox.org/git/20180724065120.7664-1-sunsh...@sunshineco.com/ I was hoping that Junio would patch-monkey that one since that crash-at-launch makes gitk unusable for Mojave users, but apparently it never got picked up. Anyhow, the issue fixed by that patch has to do with 'osascript' on Apple, and it's the only such invocation in the source code of gitk/git-gui. The 'osascript' invocation merely attempts to foreground the application at launch, so is almost certainly unrelated to the above reported behavior. Somebody running Mojave will likely need to spend some time debugging it. (Unfortunately, I'm a couple major releases behind and don't plan on upgrading.)
Re: [PATCH] i18n: fix small typos
On Wed, Nov 28, 2018 at 4:43 PM Jean-Noël Avila wrote: > Translating the new strings introduced for v2.20 showed some typos. Hard to spot by eyeball when looking at the diff, but both fixes make sense. Thanks. > Signed-off-by: Jean-Noël Avila
Re: [PATCH] tests: avoid syntax triggering old dash bug
On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
wrote:
> Avoid a bug in dash that's been fixed ever since its
> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> released with dash v0.5.7 in July 2011.
Perhaps enhance the commit message to explain the nature of the bug
itself. It is not at all obvious from reading the above or from
looking at the diff itself what the actual problem is that the patch
is fixing. (And it wasn't even immediately obvious by looking at the
commit message of ec2c84d in the dash repository.) To help readers of
this patch avoid re-introducing this problem or diagnose such a
failure, it might be a good idea to give an example of the syntax
which trips up old dash (i.e. a here-doc followed immediately by a
{...} expression) and the actual error message 'Syntax error: "}"
unexpected'.
Thanks.
> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> before URL encoding", 2016-02-09).
>
> This particular test has been failing since 5f9674243d ("config: add
> --expiry-date", 2017-11-18).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
Re: [PATCH] config.mak.dev: enable -Wpedantic in clang
On Tue, Nov 27, 2018 at 10:48 AM Carlo Arenas wrote: > On Tue, Nov 27, 2018 at 2:53 AM Eric Sunshine wrote: > > On Tue, Nov 27, 2018 at 5:06 AM Carlo Marcelo Arenas Belón > > > +ifneq ($(filter clang10,$(COMPILER_FEATURES)),) > > > +CFLAGS += -Wpedantic > > > +endif > > > > Should this condition be tightened to match only for OSX since there > > is no such clang version number in the rest world outside of Apple? > > instead, I changed it to clang4 and tested it in a FreeBSD 11 box I > was able to get a hold off, would that work better? > will update patch accordingly then Playing Devi's Advocate, what if Apple's clang "8" was, in reality, real-world clang 3? Then this condition would incorrectly enable the compiler option on Apple for a (real) clang version below 4. For this reason, it seems we shouldn't be trusting only the clang version number when dealing with Apple. (I suspect that it won't make a difference in actual practice, so it may be reasonable to punt on this issue until/if someone complains.)
Re: [PATCH] config.mak.dev: enable -Wpedantic in clang
On Tue, Nov 27, 2018 at 5:06 AM Carlo Marcelo Arenas Belón wrote: > [...] > -Wpedantic is only enabled for clang 10 or higher (only available in macOS > with latest Xcode) but this restriction should be relaxed further as more > environments are tested We know from [1] that the clang version number "10" is an Apple fiction/invention. Perhaps this commit message can be worded a bit more carefully to avoid confusing readers who are not clued in to that fact. [1]: https://public-inbox.org/git/capig+crqxq_dows2dsc1x3tagjjnwig7p4eys4kq+c2piad...@mail.gmail.com/ > Signed-off-by: Carlo Marcelo Arenas Belón > --- > +ifneq ($(filter clang10,$(COMPILER_FEATURES)),) > +CFLAGS += -Wpedantic > +endif Should this condition be tightened to match only for OSX since there is no such clang version number in the rest world outside of Apple?
Re: [PATCH v3 00/10] commit-graph write: progress output improvements
On Thu, Nov 22, 2018 at 8:28 AM Ævar Arnfjörð Bjarmason wrote: > Range-diff: > By the way, is there any way to > Pass the equivalent of "git range-diff origin/master topic-2 topic-3" > to git-format-patch? git-range-diff documentations says that the three-argument form: git range-diff is equivalent to passing two ranges: git range-diff .. .. git-format-patch synopsis shows: git format-patch --range-diff= where is the range of commits to format, and can be a range specifying the previous version, so: git format-patch --range-diff=.. .. should do what you ask. However, since the two versions in your example both derive from origin/master, you should be able to get by with the simpler: git format-patch --range-diff= .. which, if you were running git-range-diff manually, would be the equivalent of: git range-diff ... for which the range-diff machinery figures out the common base (origin/master) automatically.
Re: [PATCH v3 5/5] pretty: add support for separator option in %(trailers)
On Sun, Nov 18, 2018 at 6:45 AM Anders Waldenborg wrote:
> By default trailer lines are terminated by linebreaks ('\n'). By
> specifying the new 'separator' option they will instead be separated by
> user provided string and have separator semantics rather than terminator
> semantics. The separator string can contain the literal formatting codes
> %n and %xNN allowing it to be things that are otherwise hard to type as
> %x00, or comma and end-parenthesis which would break parsing.
>
> Signed-off-by: Anders Waldenborg
> ---
> diff --git a/Documentation/pretty-formats.txt
> b/Documentation/pretty-formats.txt
> @@ -218,9 +218,16 @@ endif::git-rev-list[]
> + ** 'separator=': specifying an alternative separator than the
> + default line feed character. SEP may can contain the literal
> + formatting codes %n and %xNN allowing it to contain characters
> + that are hard to type such as %x00, or comma and end-parenthesis
> + which would break parsing. If option is given multiple times only
> + the last one is used.
It's not clear from this documentation what constitutes a valid .
Is it restricted to a single character? Can it be an arbitrary string?
If a string, does it need to be quoted? Does it support backslash
escaping?
Although I was able to guess that %xNN allowed hex input of a 7- or
8-bit value, I found myself wondering what I was supposed to replace
'n' with in "%n". I didn't fathom that "%n" was meant to be typed
literally to specify a newline character.
> + ** Examples: `%(trailers:only,unfold,separator=%x00)` unfolds and
> + shows all trailer lines separated by NUL character,
> + `%(trailers:key=Reviewed-by,unfold)` unfolds and shows trailer
> + lines with key `Reviewed-by`.
Re: [PATCH v3 3/5] pretty: add support for "valueonly" option in %(trailers)
On Sun, Nov 18, 2018 at 6:45 AM Anders Waldenborg wrote: > With the new "key=" option to %(trailers) it often makes little sense to > show the key, as it by definition already is know which trailer is s/know/known/ > printed there. This new "valueonly" option makes it omit the key when > printing trailers. > > Signed-off-by: Anders Waldenborg
Re: [[PATCH v2] ] INSTALL: add macOS gettext and sdk header explanation to INSTALL
On Thu, Nov 15, 2018 at 11:07 PM Junio C Hamano wrote: > yanke131...@gmail.com writes: > > * add macOS gettext explanation to get the i18n locale translation take > > effect in macOS, as the most polular way of gettext > > install in macOS, the gettext is not linked by default, this commit give > > a tip on this thing. > > Also I am not quite sure what it wants to say. Perhaps you meant > to say something like this? > > Explain how to make the gettext library usable on macOS, as > with the most popular way to install it, it won't be linked > to /usr/local. > > I think the part that I had most trouble understanding was your use > of the verb "link"; it was unclear (and I am guessing) that you > meant there are missing links on the filesystem to make stuff from > gettext package available to programs that want to build with it [...] You inferred correctly, and your rewritten text conveys the needed information. > > * add macOS Command Line Tool sdk header explanation to get correct build > > in macOS 10.14+, as the CLT not install > > the header by default, we need install it by self, this commit give a way > > to install the loss headers. > > Similarly, is > > Explain how to install the Command Line Tool SDK headers > manually on macOS 10.14+ in order to correctly build Git, as > they are not installed by default. > > what you meant? Also correct. > > +In macOS, can install gettext with brew as "brew install gettext" > > +and "brew link --force gettext", the gettext is keg-only so brew not > > link > > +it to /usr/local by default, so link operation is necessary, or you can > > +follow the brew tips after install gettext. > > My best guess of what you wanted to say is > > On macOS, `gettext` can be installed with `brew install > gettext`, but because the `gettext` package is keg-only and > is not made available in `/usr/local` by default. `brew s/./,/ > link --force gettext` must be run after `brew install > gettext` to correct this to use i18n features of Git. > > but now the sentence structure is quite different and I no longer > know if that is what you meant to say. And it does not help that I > am not a Mac user. Aside from the minor punctuation issue, your rewrite correctly captures the intent and is understandable. > > If not link gettext correctly, > > +the git after build will not have correct locale translations, english > > is the > > +default language. > > If my rephrasing above is correct, then these three lines become > unnecessary, I think. Yep. > > + - In macOs 10.14, the Command Line Tool not contains sdk headers as > > before, so > > +need install Command Line Tool 10.14 and install the headers with > > command > > On macOS 10.14, the Command Line Tool no longer contains the > SDK headers; you need to also install them with the command: > > > +If not install the sdk headers correctly, git build will get errors > > blew, factly is > > +is because of this problem. > > I can guess this wants to say "without sdk headers your attempt to > build Git will blow up in your face", but not quite. > > Unless you install the SDK headers, building git will fail > with error messages like the following: Although, as you note for the other case, this sentence and the following example error message are likely unnecessary.
Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long
On Mon, Nov 12, 2018 at 3:41 AM Carlo Marcelo Arenas Belón
wrote:
> b968372279 ("read-cache: unlink old sharedindex files", 2017-03-06)
> introduced get_shared_index_expire_date using unsigned long to track
> the modification times of a shared index.
>
> dddbad728c ("timestamp_t: a new data type for timestamps", 2017-04-26)
> shows why that might problematic so move to time_t instead.
s/might/& be/
> Signed-off-by: Carlo Marcelo Arenas Belón
Re: [PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent
On Fri, Nov 9, 2018 at 5:18 AM Ævar Arnfjörð Bjarmason wrote: > Make the behavior when diff options (e.g. "--stat") are passed > consistent with how "diff" behaves. > [...] > Signed-off-by: Ævar Arnfjörð Bjarmason > --- > diff --git a/range-diff.c b/range-diff.c > @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char > *range2, > - opts.output_format |= DIFF_FORMAT_PATCH; > + if (!opts.output_format) > + opts.output_format |= DIFF_FORMAT_PATCH; I think this can just be '=' now instead of '|=' (to avoid confusing the reader, even if it's functionally equivalent).
Re: Git Reference Manual enhance
On Sat, Nov 10, 2018 at 7:21 PM Fredi Fowler wrote: > Is there any way to create pull request to git man (https://git-scm.com/docs)? That website is maintained as a project separate from Git, so you can report issues specific to the website, or create pull requests, at its project page (https://github.com/git/git-scm.com), however... > I found there some inconsistencies. For example, almost in all pages > are using [no-], but at https://git-scm.com/docs/git-merge each > command (with [no-] or without) write separately. > > There are some same inconsistencies that a easy to fix. So, if I can > sent a pull-request for such fix – inform me. Those manual pages are generated from documentation sources in the Git project, thus those fixes should be submitted to Git itself, not to the website project. Changes to Git are sent to this mailing list as patches (see Documentation/SubmittingPatches), although see gitgitgadget[1], which acts as a Github pull-request-to-Git-mailing-list gateway, as a possible alternative. [1]: https://github.com/gitgitgadget/gitgitgadget/blob/master/README.md
Re: [PATCH v1 1/1] Upcast size_t variables to uintmax_t when printing
On Fri, Nov 9, 2018 at 9:46 AM wrote: > When printing variables which contains a size, today "unsigned long" > is used at many places. > In order to be able to change the type from "unsigned long" into size_t > some day the future, we need to have a way to print 64 bit variables s/day/& in/ > on a system that has "unsigned long" defined to be 32 bit, link Win64. s/link/like/ > Upcast all those variables into uintmax_t before they are printed. > This is to prepare for a bigger change, when "unsligned long" s/unsligned/unsigned/ > will be converted into size_t for variables which may be > 4Gib. > > Signed-off-by: Torsten Bögershausen
Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options
On Thu, Nov 8, 2018 at 5:34 PM Ævar Arnfjörð Bjarmason wrote: > On Thu, Nov 08 2018, Eric Sunshine wrote: > > Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather > > than introducing this new conditional, I'm thinking that a more > > correct fix would be: > > > > opts.output_format |= DIFF_FORMAT_PATCH; > > > > (note the '|=' operator). This would result in 'opts.output_format' > > containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did > > prior to 73a834e9e2 when --no-patch was specified. > > Maybe I'm misunderstanding, but if you mean this on top: > > - if (!opts.output_format) > - opts.output_format = DIFF_FORMAT_PATCH; > + opts.output_format |= DIFF_FORMAT_PATCH; That is indeed what I mean. > Then the --stat test I've added here fails, because unlike "diff" the > "--stat" (and others) will implicitly "--patch" and you need > "--no-patch" as well (again, unlike with "diff"). This new --stat test also fails with Dscho's original git-range-diff implementation, even before 73a834e9e2 regressed the --no-patch case. The commit message seems to sell this patch as a pure regression-fix, so it feels wrong for it to be conflating a regression fix (--no-patch) with preparation for potential future improvements to other options (--stat, etc.). I could see this as a two-patch series in which patch 1/2 fixes the regression (with, say, "|="), and patch 2/2 generalizes setting 'opts.output_format' for the future. Alternately, if left as a single patch, perhaps the commit message could be fleshed out to better explain that it is doing more than merely fixing a regression (since it wasn't at all obvious to me, even after digging into the code earlier to come up with "|=", or now when trying to understand your response). > Right now --stat is pretty useless, but it could be made to make sense, > and at that point (and earlier) I think it would be confusing if > "range-diff" had different semantics with no options v.s. one option > like "--stat" v.s. "--stat -p" compared to "diff". Perhaps this sort of rationale, along with some explanatory examples could be added to the commit message to help the reader more fully understand the situation. Thanks for working on this.
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
On Wed, Nov 7, 2018 at 10:24 PM Junio C Hamano wrote: > Makefile: ease dynamic-gettext-poison transition > > Earlier we made the entire build to fail when GETTEXT_POISON=Yes is > given to make, to notify those who did not notice that text poisoning > is now a runtime behaviour. > > It turns out that this too irritating for those who need to build s/too/is &/ > and test different versions of Git that cross the boundary between > history with and without this topic to switch between two > environment variables. Demote the error to a warning, so that you > can say something like > > make GETTEXT_POISON=Yes GIT_TEST_GETTEXT_POISON test > > during the transition period, without having to worry about whether > exact version you are testing has or does not have this topic. > > Signed-off-by: Junio C Hamano
Re: [PATCH v6 1/1] http: add support selecting http version
On Thu, Nov 8, 2018 at 2:00 AM Force Charlie via GitGitGadget
wrote:
> In order to give users the freedom to control the HTTP version,
> we need to add a setting to choose which HTTP version to use.
>
> Signed-off-by: Force Charlie
> ---
> diff --git a/http.c b/http.c
> @@ -284,6 +285,9 @@ static void process_curl_messages(void)
> static int http_options(const char *var, const char *value, void *cb)
> {
> + if (!strcmp("http.version",var)) {
Style: space after comma
> + return git_config_string(_http_version, var, value);
> + }
> @@ -806,6 +834,16 @@ static CURL *get_curl_handle(void)
> +if (curl_http_version) {
> + long opt;
> + if (!get_curl_http_version_opt(curl_http_version, )) {
> + /* Set request use http version */
> + curl_easy_setopt(result, CURLOPT_HTTP_VERSION,opt);
Style: space after comma
> + }
> +}
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
On Thu, Nov 8, 2018 at 10:45 AM Johannes Schindelin wrote: > On Thu, 8 Nov 2018, Duy Nguyen wrote: > > One thing I had in mind when proposing $VARIABLE is that it opens up a > > namespace for us to expand more things (*) for example $GIT_DIR (from > > ~/.gitconfig). > > > > (*) but in a controlled way, it may look like an environment variable, > > but we only accept a selected few. And it's only expanded at the > > beginning of a path. > > I understand that desire, and I even agree. But the fact that it looks > like an environment variable, but is not, is actually a point in favor of > *not* doing this. It would violate the principle of least astonishment. Perhaps something like $:VARIABLE, which gives the convenience of $VARIABLE, but looks sufficiently different that it shouldn't trip up readers into thinking it is one. (Well-written code checking for a DOS/Windows drive letter at the beginning of a path shouldn't be confused by it.)
Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options
On Wed, Nov 7, 2018 at 7:22 AM Ævar Arnfjörð Bjarmason wrote:
> In 73a834e9e2 ("range-diff: relieve callers of low-level configuration
> burden", 2018-07-22) we broke passing down options like --no-patch,
> --stat etc. Fix that regression, and add a test for some of these
> options being passed down.
Thanks both (Ævar and Dscho) for cleaning up my mess, and sorry for
not responding sooner; I only just found time to read the discussion
thread. One comment below...
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> diff --git a/range-diff.c b/range-diff.c
> @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char
> *range2,
> memcpy(, diffopt, sizeof(opts));
> - opts.output_format = DIFF_FORMAT_PATCH;
> + if (!opts.output_format)
> + opts.output_format = DIFF_FORMAT_PATCH;
Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather
than introducing this new conditional, I'm thinking that a more
correct fix would be:
opts.output_format |= DIFF_FORMAT_PATCH;
(note the '|=' operator). This would result in 'opts.output_format'
containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did
prior to 73a834e9e2 when --no-patch was specified.
Re: [PATCH] range-diff: add a --no-patch option to show a summary
On Mon, Nov 5, 2018 at 11:17 PM Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason writes:
> > This change doesn't update git-format-patch with a --no-patch
> > option. That can be added later similar to how format-patch first
> > learned --range-diff, and then --creation-factor in
> > 8631bf1cdd ("format-patch: add --creation-factor tweak for
> > --range-diff", 2018-07-22). I don't see why anyone would want this for
> > format-patch, it pretty much defeats the point of range-diff.
>
> Does it defeats the point of range-diff to omit the patch part in
> the context of the cover letter? How?
>
> I think the output with this option is a good addition to the cover
> letter as an abbreviated form (as opposed to the full range-diff,
> whose support was added earlier) that gives an overview.
I had the same response when reading the commit message but didn't
vocalize it. I could see people wanting to suppress the 'patch' part
of the embedded range-diff in a cover letter (though probably not as
commentary in a single-patch).
> Calling this --[no-]patch might make it harder to integrate it to
> format-patch later, though. I suspect that people would expect
> "format-patch --no-patch ..." to omit both the patch part of the
> range-diff output *AND* the patch that should be applied to the
> codebase (it of course would defeat the point of format-patch, so
> today's format-patch would not pay attention to --no-patch, of
> course). We need to be careful not to break that when it happens.
Same concern on my side, which is why I was thinking of other, less
confusing, names, such as --summarize or such, though even that is too
general against the full set of git-format-patch options. It could,
perhaps be a separate option, say, "git format-patch
--range-changes=" or something, which would embed the equivalent
of "git range-diff --no-patch ..." in the cover letter.
Re: [PATCH] range-diff: add a --no-patch option to show a summary
On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason wrote: > Add a --no-patch option which shows which changes got removed, added > or moved etc., without showing the diff associated with them. This option existed in the very first version[1] of range-diff (then called branch-diff) implemented by Dscho, although it was called --no-patches (with an "es"), which it inherited from tbdiff. I think someone (possibly me) pointed out that --no-patch (sans "es") would be more consistent with existing Git options. I don't recall why Dscho removed the option during the re-rolls, but the explanation may be in that thread. I was also wondering if --summarize or --summary-only might be a better name, describing the behavior at a higher level, but since there is precedent for --no-patch (or --no-patches in tbdiff), perhaps the name is fine as is. The patch itself looks okay. [1]: https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schinde...@gmx.de/
Re: [PATCH v2 2/5] pretty: allow showing specific trailers
On Mon, Nov 5, 2018 at 3:26 AM Anders Waldenborg wrote: > Eric Sunshine writes: > > Should the code tolerate a trailing colon? (Genuine question; it's > > easy to do and would be more user-friendly.) > > I would make sense to allow the trailing colon, it is easy enough to > just strip that away when reading the argument. > > However I'm not sure how that would fit together with the possibility to > later lifting it to a regexp, hard to strip a trailing colon from a > regexp in a generic way. Which is a good reason to think about these issues now, before being set in stone. > > What happens if 'key=...' is specified multiple times? > > My first thought was to simply disallow that. But that seemed hard to > fit into current model where errors just means don't expand. > > I would guess that most useful and intuitive to user would be to handle > multiple key arguments by showing any of those keys. Agreed. > > Thinking further on the last two points, should be a regular > > expression? > > It probably would make sense. I can see how the regexp '^.*-by$' would > be useful (but glob style matching would suffice in that case). > > Also handling multi-matching with an alternation group would be elegant > %(trailers:key="(A|B)"). Except for the fact that the parser would need to > understand some kind of quoting, which seems like an major undertaking. Maybe, maybe not. As long as we're careful not to paint ourselves into a corner, it might very well be okay to start with the current implementation of matching the full key as a literal string and (perhaps much) later introduce regex as an alternate way to specify the key. For instance, 'key=literal' and 'key=/regex/' can co-exist, and the extraction of the regex inside /.../ should not be especially difficult. > I guess having it as a regular exception would also mean that it needs > to get some way to cache the re so it is compiled once, and not for each > expansion. Yes, that's something I brought up a few years ago during a GSoC project; not regex specifically, but that this parsing of the format is happening repeatedly rather than just once. I had suggested to the GSoC student that the parsing could be done early, compiling the format expression into a "machine" which could be applied repeatedly. It's a larger job, of course, not necessarily something worth tackling for your current needs. > > If I understand correctly, this is making a copy of so that it > > will be NUL-terminated since the code added to trailer.c uses a simple > > strcasecmp() to match it. Would it make sense to avoid the copy by > > adding fields 'opts.filter_key' and 'opts.filter_key_len' and using > > strncasecmp() instead? (Genuine question; not necessarily a request > > for change.) > > I'm also not very happy about that copy. > Just using strncasecmp would cause it to be prefix match, no? Well, you could retain full key match by checking for NUL explicitly with something like this: !strncasecmp(tok.buf, opts->filter_key, opts->filter_key_len) && !tok.buf[opts->filter_key_len]
Re: [PATCH v2 2/5] pretty: allow showing specific trailers
On Sun, Nov 4, 2018 at 10:48 PM Junio C Hamano wrote: > Eric Sunshine writes: > > Does the user have to include the colon when specifying of > > 'key='? > > Does 'key=', do a full or partial match on trailers? > > What happens if 'key=...' is specified multiple times? > > Thinking further on the last two points, should be a regular > > expression? > > Another thing that needs to be clarified in the document would be > case sensitivity. People sometimes spell "Signed-Off-By:" by > mistake (or is it by malice?). The documentation does say parenthetically "(matching is done case-insensitively)", so I think that's already covered. Or did you have something else in mind?
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
On Sun, Nov 4, 2018 at 6:26 PM Junio C Hamano wrote: > OK, thanks. It seems that the relative silence after this message > is a sign that the resulting patch after squashing is what everybody > is happey with? > > -- >8 -- > From: Steve Hoelzer > > Signed-off-by: Johannes Sixt > Acked-by: Steve Hoelzer It's not clear from this who the author is.
Re: [PATCH v2 2/5] pretty: allow showing specific trailers
On Sun, Nov 4, 2018 at 10:24 AM Anders Waldenborg wrote:
> Adds a new "key=X" option to "%(trailers)" which will cause it to only
> print trailers lines which matches the specified key.
>
> Signed-off-by: Anders Waldenborg
> ---
> diff --git a/Documentation/pretty-formats.txt
> b/Documentation/pretty-formats.txt
> @@ -209,11 +209,14 @@ endif::git-rev-list[]
> - %(trailers[:options]): display the trailers of the body as interpreted
>by linkgit:git-interpret-trailers[1]. The `trailers` string may be
> + followed by a colon and zero or more comma-separated options. The
> + allowed options are `only` which omits non-trailer lines from the
> + trailer block, `unfold` to make it behave as if interpret-trailer's
> + `--unfold` option was given, and `key=T` to only show trailers with
> + specified key (matching is done
> + case-insensitively).
Does the user have to include the colon when specifying of
'key='? I can see from peeking at the implementation that the
colon must not be used, but this should be documented. Should the code
tolerate a trailing colon? (Genuine question; it's easy to do and
would be more user-friendly.)
Does 'key=', do a full or partial match on trailers? And, if
partial, is the match anchored at the start or can it match anywhere
in the trailer key? I see from the implementation that it does a full
match, but this behavior should be documented.
What happens if 'key=...' is specified multiple times? Are the
multiple keys conjunctive? Disjunctive? Last-wins? I can see from the
implementation that it is last-wins, but this behavior should be
documented. (I wonder how painful it will be for people who want to
match multiple keys. This doesn't have to be answered yet, as the
behavior can always be loosened later to allow multiple-key matching
since the current syntax does not disallow such expansion.)
Thinking further on the last two points, should be a regular expression?
> + shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)`
> + unfolds and shows trailer lines with key `Reviewed-by`.
> diff --git a/pretty.c b/pretty.c
> @@ -1323,7 +1323,19 @@ static size_t format_commit_one(struct strbuf *sb, /*
> in UTF-8 */
> + opts.filter_key = xstrndup(arg, end -
> arg);
> @@ -1331,6 +1343,7 @@ static size_t format_commit_one(struct strbuf *sb, /*
> in UTF-8 */
> format_trailers_from_commit(sb, msg + c->subject_off,
> );
> }
> + free(opts.filter_key);
If I understand correctly, this is making a copy of so that it
will be NUL-terminated since the code added to trailer.c uses a simple
strcasecmp() to match it. Would it make sense to avoid the copy by
adding fields 'opts.filter_key' and 'opts.filter_key_len' and using
strncasecmp() instead? (Genuine question; not necessarily a request
for change.)
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> @@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '
> +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
> + git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual &&
> + {
> + echo "Acked-by: A U Thor " &&
> + echo
> + } >expect &&
> + test_cmp expect actual
> +'
I guess these new tests are modeled after one or two existing tests
which use a series of 'echo' statements, but an alternative would be:
cat <<-\EOF >expect &&
Acked-by: A U Thor
EOF
or, even:
test_write_lines "Acked-by: A U Thor " "" &&
though, that's probably less readable.
Re: [PATCH v2 0/5] %(trailers) improvements in pretty format
On Sun, Nov 4, 2018 at 10:23 AM Anders Waldenborg wrote: > This adds support for three new options to %(trailers): > * key -- show only trailers with specified key > * nokey -- don't show key part of trailers > * separator -- allow specifying custom separator between trailers If "key" is for including particular trailers, intuition might lead people to think that "nokey" is for excluding certain trailers. Perhaps a different name for "nokey", such as "valueonly" or "stripkey", would be better.
Re: [PATCH] tree-walk.c: fix overoptimistic inclusion in :(exclude) matching
On Sat, Nov 3, 2018 at 8:25 PM Eric Sunshine wrote: > On Sat, Nov 3, 2018 at 11:31 AM Nguyễn Thái Ngọc Duy > wrote: > > +test_expect_success 't_e_i() exclude case #8' ' > > + git init case8 && > > + ( > > + cd case8 && > > + echo file >file1 && > > + echo file >file2 && > > + git add . && > > Won't this loose git-add invocation end up adding all the junk files > from earlier tests? One might have expected to see the more restricted > invocation: > git add file1 file2 && > to make it easier to reason about the test and not worry about someone > later inserting tests above this one which might interfere with it. Upon reflection, there shouldn't be any junk files since this test is creating a new repository and "file1" and "file2" are the only files present. Apparently, I wasn't paying close enough attention when I made the earlier observation. Anyhow, the more restricted git-add invocation you used in the re-roll is still preferable since it makes the intention obvious. Thanks.
Re: [PATCH v4 2/5] am: improve author-script error reporting
On Wed, Oct 31, 2018 at 6:16 AM Phillip Wood wrote:
> diff --git a/builtin/am.c b/builtin/am.c
> @@ -308,6 +312,7 @@ static int read_author_script(struct am_state *state)
> + int i, name_i = -2, email_i = -2, date_i = -2, err = 0;
> @@ -326,14 +331,38 @@ static int read_author_script(struct am_state *state)
> + for (i = 0; i < kv.nr; i++) {
> + if (!strcmp(kv.items[i].string, "GIT_AUTHOR_NAME")) {
> + if (name_i >= 0)
> + name_i = error(_("'GIT_AUTHOR_NAME' already
> given"));
> + else
> + name_i = i;
> + }
> + ...
> + }
> + if (name_i == -2)
> + error(_("missing 'GIT_AUTHOR_NAME'"));
> + ...
> + if (date_i < 0 || email_i < 0 || date_i < 0 || err)
> goto finish;
> + state->author_name = kv.items[name_i].util;
> + ...
> retval = 0;
> finish:
> string_list_clear(, !!retval);
Junio labeled the "-2" trick "cute", and while it is optimal in that
it only traverses the key/value list once, it also increases cognitive
load since the reader has to spend a good deal more brain cycles
understanding what is going on than would be the case with simpler
(and less noisily repetitive) code.
An alternative would be to make the code trivially easy to understand,
though a bit more costly, but that expense should be negligible since
this file should always be tiny, containing very few key/value pairs,
and it's not timing critical code anyhow. For instance:
static char *find(struct string_list *kv, const char *key)
{
const char *val = NULL;
int i;
for (i = 0; i < kv.nr; i++) {
if (!strcmp(kv.items[i].string, key)) {
if (val) {
error(_("duplicate %s"), key);
return NULL;
}
val = kv.items[i].util;
}
}
if (!val)
error(_("missing %s"), key);
return val;
}
static int read_author_script(struct am_state *state)
{
...
char *name, *email, *date;
...
name = find(, "GIT_AUTHOR_NAME");
email = find(, "GIT_AUTHOR_EMAIL");
date = find(, "GIT_AUTHOR_DATE");
if (name && email && date) {
state->author_name = name;
state->author_email = email;
state->author_date = date;
retval = 0;
}
string_list_clear, !!retval);
...
Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper
'sb/filenames-with-dashes'On Fri, Nov 2, 2018 at 6:38 PM Ævar Arnfjörð
Bjarmason wrote:
> Move a 37 line for-loop mess out of "install" and into a helper
> script. This started out fairly innocent but over the years has grown
> into a hard-to-maintain monster, and my recent ad874608d8 ("Makefile:
> optionally symlink libexec/git-core binaries to bin/git", 2018-03-13)
> certainly didn't help.
>
> The shell code is ported pretty much as-is (with getopts added), it'll
> be fixed & prettified in subsequent commits.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> install_programs | 89
Pure nitpick: Earlier this year, Stefan made an effort[1] to eradicate
filenames with underscores and replace them with hyphenated filenames.
Perhaps name this "install-programs", instead.
[1]: sb/filenames-with-dashes
> diff --git a/install_programs b/install_programs
> @@ -0,0 +1,89 @@
> +while test $# != 0
> +do
> + case "$1" in
> + --X=*)
> + X="${1#--X=}"
> + ;;
> + --RM=*)
> + RM="${1#--RM=}"
> + ;;
> + --bindir=*)
> + bindir="${1#--bindir=}"
> + ;;
Is the intention that the user might have X, RM, 'bindir', etc.
already in the environment, and the switches in this script merely
override those values? Or is the intention that X, RM, 'bindir, etc.
should all start out unset? If the latter, perhaps start the script
with an initialization block which clears all these variables first:
X=
RM=
bindir=
...
Re: [RFC/PATCH 5/5] Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag
On Fri, Nov 2, 2018 at 6:38 PM Ævar Arnfjörð Bjarmason wrote:
> Let's add an option to break this backwards compatibility. Now with
> NO_INSTALL_BUILTIN_EXECDIR_ALIASES=YesPlease there's only 3 programs
> in the bindir that are hardlinked to "git" (receive-pack,
> upload-archive & upload-pack), and 3 in the
> gitexecdir (git-remote-{ftp,ftps,https} linked to git-remote-http).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> diff --git a/Makefile b/Makefile
> @@ -346,6 +346,13 @@ all::
> +# Define NO_INSTALL_BUILTIN_EXECDIR_ALIASES if you'd like to skip
> +# installing legacy such as "git-init" and "git-add" in the
> +# gitexecdir. Unless you're on a system where "which git-init" is
> +# expected to returns something set this. Users have been expected to
s/returns/return/
s/something/&,/
Although, it's not clear what "return something" means. Perhaps rephrase it as:
...git-init is expected to exist, set this.
> +# use the likes of "git init" for ages now, these programs were only
> +# provided for legacy compatibility.
> +#
Re: [RFC/PATCH 4/5] Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch
On Fri, Nov 2, 2018 at 6:38 PM Ævar Arnfjörð Bjarmason wrote:
> Add a switch for use in conjunction with the INSTALL_SYMLINKS flag
> added in ad874608d8 ("Makefile: optionally symlink libexec/git-core
> binaries to bin/git", 2018-03-13).
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> diff --git a/Makefile b/Makefile
> @@ -342,6 +342,10 @@ all::
> +# Define NO_INSTALL_SYMLINKS_FALLBACK if in conjunction with
s/if in/in/
> +# INSTALL_SYMLINKS if you'd prefer not to have the install procedure
> +# fallack on hardlinking or copying if "ln -s" fails.
> +#
Re: [PATCH] tree-walk.c: fix overoptimistic inclusion in :(exclude) matching
On Sat, Nov 3, 2018 at 11:31 AM Nguyễn Thái Ngọc Duy wrote: > Rules 8 and 18 are now updated to be less eager. We conclude that the > current entry is positively matched and included. But we say nothing > about remaining entries. tree_entry_interesting() will be called again > for those entries where we will determine entries individually. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh > @@ -194,4 +194,21 @@ test_expect_success 'multiple exclusions' ' > +test_expect_success 't_e_i() exclude case #8' ' > + git init case8 && > + ( > + cd case8 && > + echo file >file1 && > + echo file >file2 && > + git add . && Won't this loose git-add invocation end up adding all the junk files from earlier tests? One might have expected to see the more restricted invocation: git add file1 file2 && to make it easier to reason about the test and not worry about someone later inserting tests above this one which might interfere with it. > + git commit -m twofiles && > + git grep -l file HEAD :^file2 >actual && > + echo HEAD:file1 >expected && > + test_cmp expected actual && > + git grep -l file HEAD :^file1 >actual && > + echo HEAD:file2 >expected && > + test_cmp expected actual > + ) > +'
Re: [PATCH 3/3] cat-file: handle streaming failures consistently
On Tue, Oct 30, 2018 at 07:23:38PM -0400, Jeff King wrote: > There are three ways to convince cat-file to stream a blob: > > - cat-file -p $blob > > - cat-file blob $blob > > - echo $batch | cat-file --batch > > In the first two, we simply exit with the error code of > streaw_blob_to_fd(). That means that an error will cause us Your "m" got confused and ended up upside-down. > to exit with "-1" (which we try to avoid) without printing > any kind of error message (which is confusing to the user). > > Instead, let's match the third case, which calls die() on an > error. Unfortunately we cannot be more specific, as > stream_blob_to_fd() does not tell us whether the problem was > on reading (e.g., a corrupt object) or on writing (e.g., > ENOSPC). That might be an opportunity for future work, but > for now we will at least exit with a sane message and exit > code. > > Signed-off-by: Jeff King
Re: [PATCH v2] sequencer: break out of loop explicitly
On Wed, Oct 31, 2018 at 10:54 AM Johannes Schindelin wrote: > On Tue, 30 Oct 2018, Martin Ågren wrote: > > Rewrite the loop to a more idiomatic variant which doesn't muck with > > `len` in the loop body. That should help compilers and human readers > > figure out what is going on here. But do note that we need to update > > `len` since it is not only used just after this loop (where we could > > have used `i` directly), but also later in this function. > > > > Signed-off-by: Martin Ågren > > --- > > ACK. Thanks for cleaning up after me, Looks good to me, as well. Thanks for working on it.
Re: js/mingw-http-ssl, was Re: What's cooking in git.git (Oct 2018, #05; Fri, 26)
On Mon, Oct 29, 2018 at 10:10 PM Junio C Hamano wrote: > How's this? > > On platforms with recent cURL library, http.sslBackend configuration > variable can be used to choose different SSL backend at runtime. s/choose/& a/ > The Windows port uses this mechanism to switch between OpenSSL and > Secure Channel while talking over the HTTPS protocol.
Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible
On Mon, Oct 29, 2018 at 1:45 AM Nickolai Belakovski wrote: > On Sun, Oct 28, 2018 at 9:01 PM Eric Sunshine wrote: > > That said, I wouldn't necessarily oppose renaming the function, but I > > also don't think it's particularly important to do so. > > To me, I would just go lookup the signature of worktree_lock_reason > and see that it returns a pointer and I'd be satisfied with that. I > could also infer that from looking at the code if I'm just skimming > through. But if I see code like "reason = is_worktree_locked(wt)" I'm > like hold on, what's going on here?! :P I don't feel strongly about it, and, as indicated, wouldn't necessarily be opposed to it. If you do want to make that change, perhaps send it as the second patch of a 2-patch series in which patch 1 just updates the API documentation. That way, if anyone does oppose the rename in patch 2, then that patch can be dropped without having to re-send.
Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible
On Sun, Oct 28, 2018 at 9:11 PM Nickolai Belakovski
wrote:
> On Sun, Oct 28, 2018 at 4:03 PM Eric Sunshine wrote:
> > Aside from that, it doesn't seem like worktree needs any changes for
> > the ref-filter atom you have in mind. (Don't interpret this
> > observation as me being averse to changes to the API; I'm open to
> > improvements, but haven't seen anything yet indicating a bug or
> > showing that the API is more difficult than it ought to be.)
>
> You're right that these changes are not necessary in order to make a
> worktree atom.
> If there's no interest in this patch I'll withdraw it.
Withdrawing this patch seems reasonable.
> I had found it really surprising that lock_reason was not populated
> when I was accessing it while working on the worktree atom. When
> digging into it, the "internal use" comment told me nothing, both
> because there's no convention (that I'm aware of) within C to mark
> fields as such and because it fails to direct the reader to
> is_worktree_locked.
>
> How about this, I can make a patch that changes the comment next to
> lock_reason to say "/* private - use is_worktree_locked */" (choosing
> the word "private" since it's a reserved keyword in C++ and other
> languages for implementation details that are meant to be
> inaccessible) and a comment next to lock_reason_valid that just says
> "/* private */"?
A patch clarifying the "private" state of 'lock_reason' and
'lock_reason_valid' and pointing the reader at is_worktree_locked()
would be welcome.
One extra point: It might be a good idea to mention in the
documentation of is_worktree_locked() that, in addition to returning
NULL or non-NULL indicating not-locked or locked, the returned
lock-reason might very well be empty ("") when no reason was given by
the locker.
> I would also suggest renaming is_worktree_locked to
> worktree_lock_reason, the former makes me think the function is
> returning a boolean, whereas the latter more clearly conveys that a
> more detailed piece of information is being returned.
I think the "boolean"-sounding name was intentional since most
(current) callers only care about that; so, the following reads very
naturally for such callers:
if (is_worktree_locked(wt))
die(_("worktree locked; aborting"));
That said, I wouldn't necessarily oppose renaming the function, but I
also don't think it's particularly important to do so.
Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible
On Sun, Oct 28, 2018 at 5:55 PM Nickolai Belakovski > wrote: This was meant to be a reply to > https://public-inbox.org/git/cac05386f1x7tspr6kgkulwewsmdiq4vktf5rxahvzpkwbmx...@mail.gmail.com/T/#m8898c8f7c68e1ea234aca21cb2d7776b375c6f51, > please look there for some more context. I think it both did and > didn't get listed as a reply? In my mailbox I see two separate > threads but in public-inbox.org/git it looks like it correctly got > labelled as 1 thread. This whole mailing list thing is new to me, > thanks for bearing with me as I figure it out :). Gmail threads messages entirely by subject; it doesn't pay attention to In-Reply-To: or other headers for threading, which is why you see two separate threads. public-inbox.org, on the other hand, does pay attention to the headers, thus understands that all the messages belong to the same thread. Gmail's behavior may be considered anomalous. > Next time I'll make sure to change the subject line on updated > patches as PATCH v2 (that's the convention, right?). That's correct. > This is an improvement because it fixes an issue in which the fields > lock_reason and lock_reason_valid of the worktree struct were not > being populated. This is related to work I'm doing to add a worktree > atom to ref-filter.c. Those fields are considered private/internal. They are not intended to be accessed by calling code. (Unfortunately, only 'lock_reason' is thus marked; 'lock_reason_valid' should be marked "internal".) Clients are expected to retrieve the lock reason only through the provided API, is_worktree_locked(). > I see your concerns about extra hits to the filesystem when calling > get_worktrees and about users interested in lock_reason having to > make extra calls. As regards hits to the filesystem, I could remove > is_locked from the worktree struct entirely. To address the second > concern, I could refactor worktree_locked_reason to return null if > the wt is not locked. I would still want to keep is_worktree_locked > around to provide a facility to check whether or not the worktree is > locked without having to go get the reason. > > There's also been some concerns raised about caching. As I pointed > out in the other thread, the current use cases for this information > die upon accessing it, so caching is a moot point. For the use case > of a worktree atom, caching would be relevant, but it could be done > within ref-filter.c. Another option is to add the lock_reason back > to the worktree struct and have two functions for populating it: > get_worktrees_wo_lock_reason and get_worktrees_with_lock_reason. A > bit more verbose, but it makes it clear to the caller what they're > getting and what they're not getting. I might suggest starting with > doing the caching within ref-filter.c first, and if more use cases > appear for caching lock_reason we can consider the second option. It > could also be get_worktrees and get_worktrees_wo_lock_reason, though > I think most callers would be calling the latter name. > > So, my proposal for driving this patch to completion would be to: > -remove is_locked from the worktree struct > -refactor worktree_locked_reason to return null if the wt is not locked > -refactor calls to is_locked within builtin/worktree.c to call > either the refactored worktree_locked_reason or is_worktree_locked My impression, thus far, is that this all seems to be complicating rather than simplifying. These changes also seem entirely unnecessary. In [1], I made the observation that it seemed that your new ref-filter atom could be implemented with the existing is_worktree_locked() API. As far as I can tell, it can indeed be implemented without having to make any changes to the worktree API or implementation at all. The worktree API is both compact and orthogonal, and I haven't yet seen a compelling reason to change it. That said, though, the API documentation in worktree.h may be lacking, even if the implementation is not. I'll say a bit more about that below. > In addition to making the worktree code clearer, this patch fixes a > bug in which the current is_worktree_locked over-eagerly sets > lock_reason_valid. There are currently no consumers of > lock_reason_valid within master, but obviously we should fix this > before they appear :) As noted above, 'lock_reason_valid' is private/internal. It's an accident that it is not annotated such (like 'lock_reason', which is correctly annotated as "internal"). So, there should never be any external consumers of that field. It also means that there is no bug in the current code (as far as I can see) since that field is correctly consulted (internally) to determine whether the lock reason has been looked up yet. The missing "internal only" annotation is unfortunate since it may have led you down this road of considering the implementation and API broken. Moreover, the documentation for is_worktree_locked() apparently doesn't convey strongly enough that it serves the dual purpose of (1) telling you
Re: [PATCH] sequencer: clarify intention to break out of loop
On Sun, Oct 28, 2018 at 11:32 AM Martin Ågren wrote:
> [...]
> Let's be explicit about breaking out of the loop. This helps the
> compiler grok our intention. As a bonus, it might make it (even) more
> obvious to human readers that the loop stops at the first space.
This did come up in review[1,2]. I had a hard time understanding the
loop because it looked idiomatic but wasn't (and we have preconceived
notions about behavior of things which look idiomatic).
[1]:
https://public-inbox.org/git/CAPig+cQbG2s-LrAo9+7C7=dXifbWFJ3SzuNa-QePHDk7egK=j...@mail.gmail.com/
[2]:
https://public-inbox.org/git/capig+crju6nixpt2frdwz0x1hmgf1ojvzj3uk2qxege-s7i...@mail.gmail.com/
> Signed-off-by: Martin Ågren
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -2849,10 +2849,14 @@ static int do_reset(const char *name, int len, struct
> replay_opts *opts)
> /* Determine the length of the label */
> + for (i = 0; i < len; i++) {
> + if (isspace(name[i])) {
> len = i;
> + break;
> + }
> + }
> strbuf_addf(_name, "refs/rewritten/%.*s", len, name);
At least for me, this would be more idiomatic if it was coded like this:
for (i = 0; i < len; i++)
if (isspace(name[i]))
break;
strbuf_addf(..., i, name);
or, perhaps (less concise):
for (i = 0; i < len; i++)
if (isspace(name[i]))
break;
len = i;
strbuf_addf(..., len, name);
Re: [PATCH v5] branch: introduce --show-current display option
On Thu, Oct 25, 2018 at 3:04 PM Daniels Umanovskis wrote: > When called with --show-current, git branch will print the current > branch name and terminate. Only the actual name gets printed, > without refs/heads. In detached HEAD state, nothing is output. > > Signed-off-by: Daniels Umanovskis > --- > diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh > @@ -100,6 +100,50 @@ test_expect_success 'git branch -v pattern does not show > branch summaries' ' > +test_expect_success 'git branch `--show-current` works properly when tag > exists' ' > + cat >expect <<-\EOF && > + branch-and-tag-name > + EOF > + test_when_finished " > + git checkout branch-one > + git branch -D branch-and-tag-name > + " && > + git checkout -b branch-and-tag-name && > + test_when_finished "git tag -d branch-and-tag-name" && > + git tag branch-and-tag-name && If git-tag crashes before actually creating the new tag, then "git tag -d", passed to test_when_finished(), will error out too, which is probably undesirable since "cleanup code" isn't expected to error out. You could fix it this way: test_when_finished "git tag -d branch-and-tag-name || :" && git tag branch-and-tag-name && or, even better, just swap the two lines: git tag branch-and-tag-name && test_when_finished "git tag -d branch-and-tag-name" && However, do you even need to clean up the tag? Are there tests following this one which expect a certain set of tags and fail if this new one is present? If not, a simpler approach might be just to leave the tag alone (and the branch too if that doesn't need to be cleaned up). > + git branch --show-current >actual && > + test_cmp expect actual > +'
Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files
On Thu, Oct 25, 2018 at 1:47 AM Nickolai Belakovski wrote: > The motivation for the change is some work that I'm doing to add a > worktree atom in ref-filter.c. I wanted that atom to be able to access > all fields of the worktree struct and noticed that lock_reason wasn't > getting populated so I figured I'd go and fix that. > > Reviewing this work in the context of your feedback and Junio's > previous comments, I think it makes sense to only have a field in the > struct indicating whether or not the worktree is locked, and have a > separate function for getting the reason. Is your new ref-filter atom going to be boolean-only or will it also have a form (or a separate atom) for retrieving the lock-reason? I imagine both could be desirable. In any event, implementation-wise, I would think that such an atom (or atoms) could be easily built with the existing worktree API (with its lazy-loading and caching), which might be an easy way forward since you wouldn't need this patch or the updated one you posted[1], thus no need to justify such a change. > Since the only cases in > which the reason is retrieved in the current codebase are cases where > the program immediately dies, caching seems a moot point. If your new atom has a form for retrieving the lock reason, then caching could potentially be beneficial(?). [1]: https://public-inbox.org/git/20181025055142.38077-1-nbelakov...@gmail.com/
Re: [PATCH v2 1/3] [Outreachy] t3903-stash: test without configured user name
On Wed, Oct 24, 2018 at 4:06 PM Slavica Djukic wrote: > This is part of enhancement request that ask for 'git stash' to work > even if 'user.name' and 'user.email' are not configured. > Due to an implementation detail, git-stash undesirably requires > 'user.name' and 'user.email' to be set, but shouldn't. Thanks for re-rolling. This version looks better. One comment below... > Signed-off-by: Slavica Djukic > --- > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > @@ -1156,4 +1156,17 @@ test_expect_success 'stash -- works with > binary files' ' > +test_expect_failure 'stash works when user.name and user.email are not set' ' > +test_commit 1 && > +test_config user.useconfigonly true && > +test_config stash.usebuiltin true && > +sane_unset GIT_AUTHOR_NAME && > +sane_unset GIT_AUTHOR_EMAIL && > +sane_unset GIT_COMMITTER_NAME && > +sane_unset GIT_COMMITTER_EMAIL && > +test_must_fail git config user.email && Instead of simply asserting that 'user.email' is not set here, you could instead proactively ensure that it is not set. That is, instead of the test_must_fail(), do this: test_unconfig user.email && test_unconfig user.name && > +echo changed >1.t && > +git stash > +' > + > test_done > -- > 2.19.1.windows.1 >
Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files
On Wed, Oct 24, 2018 at 2:39 AM wrote: > lock_reason is now populated during the execution of get_worktrees > > is_worktree_locked has been simplified, renamed, and changed to internal > linkage. It is simplified to only return the lock reason (or NULL in case > there is no lock reason) and to not have any side effects on the inputs. > As such it made sense to rename it since it only returns the reason. > > Since this function was now being used to populate the worktree struct's > lock_reason field, it made sense to move the function to internal > linkage and have callers refer to the lock_reason field. The > lock_reason_valid field was removed since a NULL/non-NULL value of > lock_reason accomplishes the same effect. > > Some unused variables within worktree source code were removed. Thanks for the submission. One thing which isn't clear from this commit message is _why_ this change is desirable at this time, aside from the obvious simplification of the code and client interaction (or perhaps those are the _why_?). Although I had envisioned populating the "reason" field greedily in the way this patch does, not everyone agrees that doing so is desirable. In particular, Junio argued[1,2] for populating it lazily, which accounts for the current implementation. That's why I ask about the _why_ of this change since it will likely need to be justified in a such a way to convince Junio to change his mind. Thanks. [1]: https://public-inbox.org/git/xmqq8tyq5czn@gitster.mtv.corp.google.com/ [2]: https://public-inbox.org/git/xmqq4m9d0w6v@gitster.mtv.corp.google.com/
Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name
On Tue, Oct 23, 2018 at 12:31 PM Slavica wrote: > This is part of enhancement request that ask for `git stash` to work even if > `user.name` is not configured. > The issue is discussed here: > https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u. As Christian mentioned already, it's best to try to describe the issue succinctly in the commit message so readers can understand it without chasing a link. For this simple case, it should be sufficient to explain that, due to an implementation detail, git-stash undesirably requires 'user.name' and 'user.email' to be set, but shouldn't. > Signed-off-by: Slavica > --- > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > @@ -1156,4 +1156,21 @@ test_expect_success 'stash -- works with > binary files' ' > +test_expect_failure 'stash with HOME as non-existing directory' ' The purpose of this test is to demonstrate that git-stash has an undesirable requirement that 'user.name' and 'user.email' be set. The test title should reflect that. So, instead of talking about non-existent HOME (which is just an implementation detail of the test), a better test title would be something like "stash works when user.name and user.email are not set". > +test_commit 1 && > +test_config user.useconfigonly true && > +test_config stash.usebuiltin true && > +( > +HOME=$(pwd)/none && > +export HOME && > +unset GIT_AUTHOR_NAME && Use sane_unset() for all of these rather than bare 'unset'. > +unset GIT_AUTHOR_EMAIL && > +unset GIT_COMMITTER_NAME && > +unset GIT_COMMITTER_EMAIL && > +test_must_fail git config user.email && > +echo changed >1.t && > + git stash Christian already mentioned the odd indentation. > +) > +'
Re: [PATCH 5/5] t7501: rename commit test to comply with naming convention
On Mon, Oct 22, 2018 at 11:54 PM Stephen P. Smith wrote: > The naming convention was documented [1] but this script was not > renamed. > > The original commit message indicates the script tests basic commit > functionality. Clean up the test name by changing the file name to > specify the intent as documented in the initial commit. > > Signed-off-by: Stephen P. Smith > --- > diff --git a/t/t7501-commit.sh b/t/t7501-commit-basic-funtionality.sh > rename from t/t7501-commit.sh > rename to t/t7501-commit-basic-funtionality.sh s/funtionality/functionality/
Re: [PATCH 4/5] t7500: rename commit tests script to comply with naming convention
On Mon, Oct 22, 2018 at 11:53 PM Stephen P. Smith wrote: > When the test naming convention was documented[1] the commit script > was not renamed. > > Update the test description to note that the tests fall into for > general categories: template, sign-off, -F and squash tests. s/for/four/ > Chose to not add "File" to the new script name as that did not seem to > convey the current test contents for that switch. > > Signed-off-by: Stephen P. Smith
Re: [PATCH 2/5] t7509: cleanup description and filename
On Mon, Oct 22, 2018 at 11:53 PM Stephen P. Smith wrote:> > Rename test and update the test description to explicitly state that > included tests all relate to commit authorship. The t7509-commit.sh > file was not rnemamed when other scripts were updated in compliance s/rnemamed/renamed/ > with the test naming convention. > > Signed-off-by: Stephen P. Smith
Re: [PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash
On Mon, Oct 22, 2018 at 6:15 PM Johannes Schindelin via GitGitGadget
wrote:
> When `git stash apply ` sees an argument that consists only of
> digits, it tries to be smart and interpret it as `stash@{}`.
>
> Unfortunately, an all-digit hash (which is unlikely but still possible)
> is therefore misinterpreted as `stash@{}` reflog.
>
> To prevent that from happening, let's append `^0` after the stash hash,
> to make sure that it is interpreted as an OID rather than as a number.
>
> Signed-off-by: Johannes Schindelin
> ---
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> @@ -253,6 +253,8 @@ static int apply_autostash(struct rebase_options *opts)
>
> if (read_one(path, ))
> return error(_("Could not read '%s'"), path);
> + /* Ensure that the hash is not mistake for a number */
s/mistake/mistaken/
Re: [PATCH 1/6] doc: clarify boundaries of 'git worktree list --porcelain'
On Mon, Oct 22, 2018 at 4:46 PM Andreas Heiduk wrote: > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > @@ -270,8 +270,8 @@ Porcelain Format > The porcelain format has a line per attribute. Attributes are listed with a > label and value separated by a single space. Boolean attributes (like 'bare' > and 'detached') are listed as a label only, and are only present if and only > -if the value is true. An empty line indicates the end of a worktree. For > -example: > +if the value is true. The first attribute of a worktree is always > `worktree`, > +an empty line indicates the end of the record. For example: When I suggested the --porcelain option for "git worktree list" and provided an example of its proposed output, the idea all along was that the "worktree" line itself would indicate start-of-stanza. A blank line between records is superfluous and unnecessary. Unfortunately, by the time the implementation was posted with blank line as stanza separator, I was not around to contest it, and it ended up in a release, after which point it was too late to change it. So, the tl;dr is that this documentation update agrees with the original intention as I envisioned it (although I wouldn't be sad to see the mention of "blank line" dropped altogether, but that's perhaps a separate battle).
Re: [PATCH v2 2/3] reset: add new reset.quiet config setting
On Fri, Oct 19, 2018 at 12:46 PM Jeff King wrote: > On Fri, Oct 19, 2018 at 12:36:44PM -0400, Eric Sunshine wrote: > > How does the user reverse this for a particular git-reset invocation? > > There is no --no-quiet or --verbose option. > > > > Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in > > builtin/reset.c and document that --verbose overrides --quiet and > > reset.quiet (or something like that). > > I think OPT__QUIET() provides --no-quiet, since it's really an > OPT_COUNTUP() under the hood. Saying "--no-quiet" should reset it back > to 0. Okay. In any case, --no-quiet probably ought to be mentioned alongside the "reset.quiet" option (and perhaps in git-reset.txt to as a way to reverse "reset.quiet").
Re: [PATCH v2 2/3] reset: add new reset.quiet config setting
On Fri, Oct 19, 2018 at 12:12 PM Ben Peart wrote: > Add a reset.quiet config setting that sets the default value of the --quiet > flag when running the reset command. This enables users to change the > default behavior to take advantage of the performance advantages of > avoiding the scan for unstaged changes after reset. Defaults to false. > > Signed-off-by: Ben Peart > --- > diff --git a/Documentation/config.txt b/Documentation/config.txt > @@ -2728,6 +2728,9 @@ rerere.enabled:: > +reset.quiet:: > + When set to true, 'git reset' will default to the '--quiet' option. How does the user reverse this for a particular git-reset invocation? There is no --no-quiet or --verbose option. Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in builtin/reset.c and document that --verbose overrides --quiet and reset.quiet (or something like that).
Re: [PATCH] send-email: explicitly disable authentication
On Thu, Oct 18, 2018 at 6:02 PM Joshua Watt wrote: > On Thu, 2018-10-18 at 17:53 -0400, Eric Sunshine wrote: > > On Thu, Oct 18, 2018 at 5:16 PM Joshua Watt > > wrote: > > Implementation complexity aside, spelling the option --no-smtp-auth > > might be more intuitive and consistent than --smtp-auth=none. > > One advantage of --smtp-auth=none is that it can also be done with a > config variable sendemail.smtpauth="none". Would be also add a config > variable like sendemail.nosmtpauth (the negative seems strange to me)? I would not expect to see a "negating" config variable like that. I was just suggesting that a "--no-*" command-line option might be more intuitive. > Or maybe --no-smtp-auth is just a shorthand alias for --smtp-auth=none? That's one possibility.
Re: [PATCH] receive: denyCurrentBranch=updateinstead should not blindly update
On Fri, Oct 19, 2018 at 12:57 AM Junio C Hamano wrote: > The handling of receive.denyCurrentBranch=updateInstead was added to > a switch statement that handles other values of the variable, but > all the other case arms only checked a condition to reject the > attempted push, or let later logic in the same function to still > intervene, so that a push that does not fast-forward (which is > checked after the switch statement in question) is still rejected. > > But the handling of updateInstead incorrectly took immediate effect, > without giving other checks a chance to intervene. > > Instead of calling update_worktree() that causes the side effect > immediately, just note the fact that we will need to call the > funciton later, and first give other checks chance to reject the s/funciton/function s/chance/a &/ > request. After the update-hook gets a chance to reject the push > (which happens as the last step in a series of checks), call > update_worktree() when we earlier detected the need to. > > Reported-by: Rajesh Madamanchi > Signed-off-by: Junio C Hamano
Re: [PATCH] send-email: explicitly disable authentication
On Thu, Oct 18, 2018 at 5:16 PM Joshua Watt wrote: > It can be necessary to disable SMTP authentication by a mechanism other > than sendemail.smtpuser being undefined. For example, if the user has > sendemail.smtpuser set globally but wants to disable authentication > locally in one repository. > > --smtp-auth and sendemail.smtpauth now understand the value 'none' which > means to disable authentication completely, even if an authentication > user is specified. Implementation complexity aside, spelling the option --no-smtp-auth might be more intuitive and consistent than --smtp-auth=none. > The value 'none' is lower case to avoid conflicts with any RFC 4422 > authentication mechanisms. > > Signed-off-by: Joshua Watt
Re: [PATCH v4] branch: introduce --show-current display option
On Thu, Oct 18, 2018 at 5:51 AM Johannes Schindelin wrote: > On Wed, 17 Oct 2018, Eric Sunshine wrote: > > On Wed, Oct 17, 2018 at 6:18 AM Johannes Schindelin > > wrote: > > > My suspicion: it is essentially the `(exit 117)` that adds about 100ms to > > > every of those 67 test cases. > > > > The subshell chain-linter adds a 'sed' and 'grep' invocation to each test > > which doesn't help. (v1 of the subshell chain-linter only added a 'sed', > > but that changed with v2.) > > You could disable the subshell chain-linter like this if you want test the > > (exit 117) goop in isolation: > > You're right! This is actually responsible for about five of those seven > seconds. The subshell still hurts a little, as it means that every single > of the almost 20,000 test cases we have gets slowed down by ~0.03s, which > amounts to almost 10 minutes. > > This is "only" for the Windows phase of our Continuous Testing, of course. > Yet I think we can do better than this. > > How difficult/involved, do you think, would it be to add a t/helper/ > command for chain linting? Probably more effort than it's worth, and it would only save one process invocation. Since the subshell portion of the chain-linting is done by pure textual inspection, an alternative I had considered was to just perform it as a preprocess over the entire test suite, much like the other t/Makefile "test-lint" targets. In other words, the entire test suite might be tested in one go with something like this: sed -f chainlint.sed t*.sh | grep -q '?![A-Z][A-Z]*?!' && echo "BROKEN &&-chain" That won't work today since chainlint.sed isn't written to understand everything which we might see outside of a test_expect_*, but doing it that way is within the realm of possibility. There were two reasons why I didn't pursue that approach. First, although I was expecting Windows folks to complain (or at least speak up) about the extra 'sed' and 'grep', nobody did, so my impression was that those two extra commands were likely lost in the noise of the rest of the boilerplate commands invoked by test_expect_success(), test_run_(), test_eval_(), etc., and by whatever expensive commands are invoked by each test itself. Second, the top-level &&-chain "(exit 117)" linting kicks in even when you run a single test script manually, say after editing a test, which is exactly when you want to discover that you botched a &&-chain, so it seemed a good idea for the subshell &&-chain linter to follow suit. The t/Makefile "test-lint" targets, on the other hand, don't kick in when running test scripts in isolation. However, a pragmatic way to gain back those 10 minutes might be simply to disable the chain-linter for continuous integration testing on Windows, but leave it enabled on other platforms. This way, we'd still catch broken &&-chains, with the exception of tests which are specific to Windows, of which I think there are very few.
Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting
On Wed, Oct 17, 2018 at 12:40 PM Ben Peart wrote: > Add a reset.quietDefault config setting that sets the default value of the > --quiet flag when running the reset command. This enables users to change > the default behavior to take advantage of the performance advantages of > avoiding the scan for unstaged changes after reset. Defaults to false. As with the previous patch, my knee-jerk reaction is that this really feels wrong being tied to --quiet. It's particularly unintuitive. What I _could_ see, and what would feel more natural is if you add a new option (say, --optimize) which is more general, incorporating whatever optimizations become available in the future, not just this one special-case. A side-effect of --optimize is that it implies --quiet, and that is something which can and should be documented. > Signed-off-by: Ben Peart
Re: [PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet
On Wed, Oct 17, 2018 at 12:40 PM Ben Peart wrote: > When git reset is run with the --quiet flag, don't bother finding any > additional unstaged changes as they won't be output anyway. This speeds up > the git reset command by avoiding having to lstat() every file looking for > changes that aren't going to be reported anyway. > > Signed-off-by: Ben Peart > --- > diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt > @@ -95,7 +95,9 @@ OPTIONS > --quiet:: > - Be quiet, only report errors. > + Be quiet, only report errors. Can optimize the performance of reset > + by avoiding scaning all files in the repo looking for additional > + unstaged changes. s/scaning/scanning/ However, I'm not convinced that this should be documented here or at least in this fashion. When I read this new documentation before reading the commit message, I was baffled by what it was trying to say since --quiet'ness is a superficial quality, not an optimizer. My knee-jerk reaction is that it doesn't belong in end-user documentation at all since it's an implementation detail, however, I can see that such knowledge could be handy for people in situations which would be helped by this. That said, if you do document it, this doesn't feel like the correct place to do so; it should be in a "Discussion" section or something. (Who would expect to find --quiet documentation talking about optimizations? Likely, nobody.)
Re: [PATCH v4] branch: introduce --show-current display option
On Wed, Oct 17, 2018 at 6:18 AM Johannes Schindelin
wrote:
> I realized yesterday that the &&-chain linting we use for every single
> test case takes a noticeable chunk of time:
>
> $ time ./t0006-date.sh --quiet
> real0m20.973s
> $ time ./t0006-date.sh --quiet --no-chain-lint
> real0m13.607s
>
> My suspicion: it is essentially the `(exit 117)` that adds about 100ms to
> every of those 67 test cases.
The subshell chain-linter adds a 'sed' and 'grep' invocation to each test which
doesn't help. (v1 of the subshell chain-linter only added a 'sed', but that
changed with v2.)
> With that in mind, I would like to suggest that we should start to be very
> careful about using subshells in our test suite.
You could disable the subshell chain-linter like this if you want test the
(exit 117) goop in isolation:
--- 8< ---
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3f95bfda60..48323e503c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -675,8 +675,7 @@ test_run_ () {
trace=
# 117 is magic because it is unlikely to match the exit
# code of other programs
- if $(printf '%s\n' "$1" | sed -f
"$GIT_BUILD_DIR/t/chainlint.sed" | grep -q '?![A-Z][A-Z]*?!') ||
- test "OK-117" != "$(test_eval_ "(exit 117) &&
$1${LF}${LF}echo OK-\$?" 3>&1)"
+ if test "OK-117" != "$(test_eval_ "(exit 117) &&
$1${LF}${LF}echo OK-\$?" 3>&1)"
then
error "bug in the test script: broken &&-chain or
run-away HERE-DOC: $1"
fi
--- 8< ---
Re: [PATCH v4] branch: introduce --show-current display option
On Tue, Oct 16, 2018 at 7:09 PM Junio C Hamano wrote:
> Eric Sunshine writes:
> > This cleanup "checkout" needs to be encapsulated within a
> > test_when_finished(), doesn't it? Preferably just after the "git
> > checkout -b" invocation.
>
> In the meantime, here is what I'll have in 'pu' on top.
>
> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> @@ -119,12 +119,14 @@ test_expect_success 'git branch `--show-current` works
> properly when tag exists'
> cat >expect <<-\EOF &&
> branch-and-tag-name
> EOF
> - test_when_finished "git branch -D branch-and-tag-name" &&
> + test_when_finished "
> + git checkout branch-one
> + git branch -D branch-and-tag-name
> + " &&
> git checkout -b branch-and-tag-name &&
> test_when_finished "git tag -d branch-and-tag-name" &&
> git tag branch-and-tag-name &&
> git branch --show-current >actual &&
> - git checkout branch-one &&
> test_cmp expect actual
> '
This make sense to me.
> @@ -137,8 +139,7 @@ test_expect_success 'git branch `--show-current` works
> properly with worktrees'
> git worktree add worktree branch-two &&
> (
> git branch --show-current &&
> - cd worktree &&
> - git branch --show-current
> + git -C worktree branch --show-current
> ) >actual &&
> test_cmp expect actual
> '
The subshell '(...)' could become '{...}' now that the 'cd' is gone,
but that's a minor point.
Re: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name
On Tue, Oct 16, 2018 at 8:38 AM Johannes Schindelin
wrote:
> On Mon, 15 Oct 2018, Eric Sunshine wrote:
> > On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget
> > wrote:
> > > + len = ARRAY_SIZE(wbuffer);
> > > + if (GetUserNameExW(type, wbuffer, )) {
> > > + char *converted = xmalloc((len *= 3));
>
> Oh wow. I *just* realized this. It is an off-by-one: the `xmalloc()` call
> needs to receive `len + 1` to accommodate for the trailing NUL. Will fix,
> too.
Or, xmallocz() (note the "z") which gives you overflow-checking of the
+1 for free.
Re: [PATCH v2 12/13] README: add a build badge (status of the Azure Pipelines build)
On Mon, Oct 15, 2018 at 6:12 AM Johannes Schindelin via GitGitGadget wrote: > Just like so many other OSS projects, we now also have a build badge. > > Signed-off-by: Johannes Schindelin > --- > diff --git a/README.md b/README.md > @@ -1,3 +1,5 @@ > +[](https://dev.azure.com/git/git/_build/latest?definitionId=2) The first URL is broken "https:/..." rather than "https://...;.
Re: [PATCH v2 09/13] git-p4: use `test_atexit` to kill the daemon
On Mon, Oct 15, 2018 at 6:12 AM Johannes Schindelin via GitGitGadget
wrote:
> This should be more reliable than the current method, and prepares the
> test suite for a consistent way to clean up before re-running the tests
> with different options.
>
> Signed-off-by: Johannes Schindelin
> ---
> diff --git a/t/t-basic.sh b/t/t-basic.sh
> @@ -134,6 +134,7 @@ check_sub_test_lib_test_err () {
> +cat >/dev/null <<\DDD
> test_expect_success 'pretend we have a fully passing test suite' "
> run_sub_test_lib_test full-pass '3 passing tests' <<-\\EOF &&
> for i in 1 2 3
> @@ -820,6 +821,7 @@ test_expect_success 'tests clean up even on failures' "
> > 1..2
> EOF
> "
> +DDD
Is this "DDD" here-doc leftover debugging goop?
Re: [PATCH 3/3] mingw: use domain information for default email
On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget
wrote:
> When a user is registered in a Windows domain, it is really easy to
> obtain the email address. So let's do that.
> [...]
> Signed-off-by: Johannes Schindelin
> ---
> diff --git a/compat/mingw.c b/compat/mingw.c
> @@ -1826,6 +1826,11 @@ static char *get_extended_user_info(enum
> EXTENDED_NAME_FORMAT type)
> +char *mingw_query_user_email(void)
> +{
> + return get_extended_user_info(NameUserPrincipal);
> +}
> diff --git a/ident.c b/ident.c
> @@ -168,6 +168,9 @@ const char *ident_default_email(void)
> + } else if ((email = query_user_email()) && email[0]) {
> + strbuf_addstr(_default_email, email);
> + free((char *)email);
If query_user_email(), which calls get_extended_user_info(), returns
NULL, then we do nothing (and don't worry about freeing the result).
However, if query_user_email() returns a zero-length allocated string,
then this conditional will leak that string (due to the email[0]
check). From patch 2/3, we see in get_extended_user_info():
+static char *get_extended_user_info(enum EXTENDED_NAME_FORMAT type)
+{
+ if (GetUserNameExW(type, wbuffer, )) {
+ char *converted = xmalloc((len *= 3));
+ if (xwcstoutf(converted, wbuffer, len) >= 0)
+ return converted;
that it may possibly return a zero-length string (due to the ">=0").
Do we care about this potential leak (or is GetUserNameExW()
guaranteed never to return such a string)?
Re: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name
On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget
wrote:
> We do have the excellent GetUserInfoEx() function to obtain more
> detailed information of the current user (if the user is part of a
> Windows domain); Let's use it.
> [...]
> Signed-off-by: Johannes Schindelin
> ---
> diff --git a/compat/mingw.c b/compat/mingw.c
> @@ -1798,6 +1799,33 @@ int mingw_getpagesize(void)
> +static char *get_extended_user_info(enum EXTENDED_NAME_FORMAT type)
> +{
> + DECLARE_PROC_ADDR(secur32.dll, BOOL, GetUserNameExW,
> + enum EXTENDED_NAME_FORMAT, LPCWSTR, PULONG);
> + static wchar_t wbuffer[1024];
Does this need to be static? It's not being returned to the caller.
> + len = ARRAY_SIZE(wbuffer);
> + if (GetUserNameExW(type, wbuffer, )) {
> + char *converted = xmalloc((len *= 3));
> + if (xwcstoutf(converted, wbuffer, len) >= 0)
> + return converted;
> + free(converted);
> + }
If xwcstoutf() fails, 'converted' is freed; otherwise, the allocated
'converted' is stored in the caller's statically held 'passwd' struct.
Okay.
> + return NULL;
> +}
Re: [PATCH 1/3] getpwuid(mingw): initialize the structure only once
On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget
wrote:
> Signed-off-by: Johannes Schindelin
> ---
> diff --git a/compat/mingw.c b/compat/mingw.c
> @@ -1800,16 +1800,27 @@ int mingw_getpagesize(void)
> struct passwd *getpwuid(int uid)
> {
> + static unsigned initialized;
> static char user_name[100];
> - static struct passwd p;
> + static struct passwd *p;
>
> + if (initialized)
> + return p;
> +
> + len = sizeof(user_name);
> + if (!GetUserName(user_name, )) {
> + initialized = 1;
> return NULL;
> + }
If GetUserName() fails, that failure is recorded (as "initialized=1"
and 'p' is still NULL), so subsequent invocations just return NULL
without doing any more work. Makes sense.
> + p = xmalloc(sizeof(*p));
> + p->pw_name = user_name;
> + p->pw_gecos = "unknown";
> + p->pw_dir = NULL;
> +
> + initialized = 1;
> + return p;
> }
Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL
On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
wrote:
> This config value is only used if http.sslBackend is set to "schannel",
> which forces cURL to use the Windows Certificate Store when validating
> server certificates associated with a remote server.
>
> This is only supported in cURL 7.44 or later.
> [...]
> Signed-off-by: Brendan Forster
> ---
> diff --git a/http.c b/http.c
> @@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
> + if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
> + !http_schannel_check_revoke) {
> +#if LIBCURL_VERSION_NUM >= 0x072c00
> + curl_easy_setopt(result, CURLOPT_SSL_OPTIONS,
> CURLSSLOPT_NO_REVOKE);
> +#else
> + warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options
> because\n"
> + "your curl version is too old (>= 7.44.0)");
This message is confusing. If your curl is too old, shouldn't the ">=" be a "<"?
> +#endif
> + }
Re: [PATCH 1/3] http: add support for selecting SSL backends at runtime
On Mon, Oct 15, 2018 at 6:14 AM Johannes Schindelin via GitGitGadget
wrote:
> This patch adds the Git side of that feature: by setting http.sslBackend
> to "openssl" or "schannel", Git for Windows can now choose the SSL
> backend at runtime.
> [...]
> Signed-off-by: Johannes Schindelin
> ---
> diff --git a/http.c b/http.c
> @@ -995,6 +1003,33 @@ void http_init(struct remote *remote, const char *url,
> int proactive_auth)
> +#if LIBCURL_VERSION_NUM >= 0x073800
> + if (http_ssl_backend) {
> + const curl_ssl_backend **backends;
> + struct strbuf buf = STRBUF_INIT;
> + int i;
> +
> + switch (curl_global_sslset(-1, http_ssl_backend, )) {
> + case CURLSSLSET_UNKNOWN_BACKEND:
> + strbuf_addf(, _("Unsupported SSL backend '%s'. "
> + "Supported SSL backends:"),
> + http_ssl_backend);
> + for (i = 0; backends[i]; i++)
> + strbuf_addf(, "\n\t%s",
> backends[i]->name);
> + die("%s", buf.buf);
This is the only 'case' arm which utilizes 'strbuf buf', and it
die()s, so no leak despite a lack of strbuf_release(). Okay.
> + case CURLSSLSET_NO_BACKENDS:
> + die(_("Could not set SSL backend to '%s': "
> + "cURL was built without SSL backends"),
> + http_ssl_backend);
> + case CURLSSLSET_TOO_LATE:
> + die(_("Could not set SSL backend to '%s': already
> set"),
> + http_ssl_backend);
> + case CURLSSLSET_OK:
> + break; /* Okay! */
> + }
> + }
> +#endif
Re: [PATCH v4] branch: introduce --show-current display option
On Fri, Oct 12, 2018 at 9:34 AM Daniels Umanovskis wrote: > When called with --show-current, git branch will print the current > branch name and terminate. Only the actual name gets printed, > without refs/heads. In detached HEAD state, nothing is output. > > Signed-off-by: Daniels Umanovskis > --- > diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh > @@ -100,6 +100,49 @@ test_expect_success 'git branch -v pattern does not show > branch summaries' ' > +test_expect_success 'git branch `--show-current` works properly when tag > exists' ' > + cat >expect <<-\EOF && > + branch-and-tag-name > + EOF > + test_when_finished "git branch -D branch-and-tag-name" && > + git checkout -b branch-and-tag-name && > + test_when_finished "git tag -d branch-and-tag-name" && > + git tag branch-and-tag-name && > + git branch --show-current >actual && > + git checkout branch-one && This cleanup "checkout" needs to be encapsulated within a test_when_finished(), doesn't it? Preferably just after the "git checkout -b" invocation. > + test_cmp expect actual > +'
Re: [PATCH 2/3] add read_author_script() to libgit
On Wed, Oct 10, 2018 at 6:14 AM Phillip Wood wrote: > On 14/09/2018 00:49, Eric Sunshine wrote: > > What if, instead of exit()ing directly, you drop the conditional and > > instead return the value of read_author_script(): > > > > return read_author_script(...); > > > > and let the caller deal with the zero or non-zero return value as > > usual? (True, you'd get two error messages upon failure rather than > > just one, but that's not necessarily a bad thing.) > > > > A possibly valid response is that such a change is outside the scope > > of this series since the original code shared this odd architecture of > > sometimes returning 0, sometimes -1, and sometimes die()ing. > > My aim was to try and to keep the changes to a minimum as this patch > isn't about changing the odd architecture of the original. I could add a > follow up patch that cleans things up as you suggest. The code already had that weird mix of return(s) and die(), hence the state is no worse after this patch than before. So, a cleanup patch later in the series, a follow up series, or doing nothing at all are all reasonable approaches. I don't insist on it for this patch series. Thanks.
Re: [PATCH v2 1/2] rebase -i: clarify what happens on a failed `exec`
On Wed, Oct 10, 2018 at 4:54 AM Johannes Schindelin via GitGitGadget wrote: > We had not documented previously what happens when an `exec` command in > an interactive rebase fails. Now we do. > > Signed-off-by: Johannes Schindelin > --- > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > @@ -420,7 +420,8 @@ idea unless you know what you are doing (see BUGS below). > --exec :: > Append "exec " after each line creating a commit in the > final history. will be interpreted as one or more shell > - commands. > + commands. Anz command that fails will interrupt the rebase, > + withe exit code 1. s/Anz/Any/ s/withe/with/
Re: [PATCH 0/2] branch: introduce --current display option
On Wed, Oct 10, 2018 at 5:29 AM Eric Sunshine wrote: > On Tue, Oct 9, 2018 at 4:59 PM Junio C Hamano wrote: > > My inclination is to recommend to: > > > > (1) name the "show the current one" not "--current" but with some > > verb > > > > (2) display nothing when there is no current branch (i.e. detached > > HEAD) and without any error. > > Sensible suggestions. Also, please documentation any new option(s) in > Documentation/git-branch.txt. Sorry, I was expecting to see the documentation update in patch 1 and didn't notice that it was being done by patch 2. The reason I had that expectation is that a change of functionality and the documentation of that change are logically related, thus (usually) ought to be presented together. Therefore, when you re-roll, you may want to consider squashing the two patches into one.
Re: [PATCH 0/2] branch: introduce --current display option
On Tue, Oct 9, 2018 at 4:59 PM Junio C Hamano wrote: > My inclination is to recommend to: > > (1) name the "show the current one" not "--current" but with some > verb > > (2) display nothing when there is no current branch (i.e. detached > HEAD) and without any error. Sensible suggestions. Also, please documentation any new option(s) in Documentation/git-branch.txt.
Re: [PATCH 02/14] builtin/repack: replace hard-coded constant
On Mon, Oct 8, 2018 at 6:27 PM Stefan Beller wrote:
> On Mon, Oct 8, 2018 at 2:57 PM brian m. carlson
> wrote:
> > - if (line.len != 40)
> > - die("repack: Expecting 40 character sha1 lines only
> > from pack-objects.");
> > + if (line.len != the_hash_algo->hexsz)
> > + die("repack: Expecting full hex object ID lines
> > only from pack-objects.");
>
> This is untranslated as it is plumbing? If so, maybe
>
> if (is_sha1(the_hash_algo)
> die("repack: Expecting 40 character sh...
> else
> die(repack: Expecting full hex object ID in %s, of length %d",
> the_hash_algo->name,
> the_hash_algo->hexsz);
Special-casing for SHA-1 seems overkill for an error message. A script
expecting this particular error condition and this particular error
message would be fragile indeed.
Re: [PATCH v2 8/8] reflog expire: cover reflog from all worktrees
On Tue, Oct 2, 2018 at 12:16 PM Duy Nguyen wrote: > On Sun, Sep 30, 2018 at 7:36 AM Eric Sunshine wrote: > > On Sat, Sep 29, 2018 at 3:11 PM Nguyễn Thái Ngọc Duy > > wrote: > > > +--single-worktree:: > > > + By default when `--all` is specified, reflogs from all working > > > + trees are processed. This option limits the processing to reflogs > > > + from the current working tree only. > > > > Bikeshedding: I wonder if this should be named "--this-worktree" or > > "--this-worktree-only" or if it should somehow be orthogonal to --all > > rather than modifying it. (Genuine questions. I don't have the > > answers.) > > It follows a precedent (made by me :p) which is rev-list > --single-worktree. I doubt that option is widely used though so we > could still rename it if there's a better name. I made > --single-worktree to contrast "all worktrees" by default. Even if it's > "this/current worktree" it still has to somehow say "everything in > this worktree" so I felt modifying --all was a good idea. Precedent overrides bikeshedding, so leaving it as-is is fine.
Re: [PATCH v3 1/2] t1300: extract and use test_cmp_config()
On Tue, Oct 2, 2018 at 12:07 PM Nguyễn Thái Ngọc Duy wrote:
> In many config-related tests it's common to check if a config variable
> has expected value and we want to print the differences when the test
> fails. Doing it the normal way is three lines of shell code. Let's add
> a function do to all this (and a little more).
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -747,6 +747,29 @@ test_cmp() {
> +test_cmp_config() {
> + local GD
If you re-roll, maybe make this part of the &&-chain to protect
against someone coming along and inserting code above this line
without realizing that the &&-chain is broken.
> + if test "$1" = "-C"
> + then
> + shift &&
> + GD="-C $1" &&
> + shift
> + fi &&
> + printf "%s\n" "$1" >expect.config &&
> + shift &&
> + git $GD config "$@" >actual.config &&
> + test_cmp expect.config actual.config
> +}
Re: [PATCH v2 2/2] worktree: add per-worktree config files
On Sun, Sep 30, 2018 at 3:16 AM Duy Nguyen wrote:
> On Sun, Sep 30, 2018 at 6:33 AM Eric Sunshine wrote:
> > > diff --git a/builtin/config.c b/builtin/config.c
> > > @@ -645,7 +649,20 @@ int cmd_config(int argc, const char **argv, const
> > > char *prefix)
> > > + else if (use_worktree_config) {
> > > + struct worktree **worktrees = get_worktrees(0);
> > > + if (repository_format_worktree_config)
> > > + given_config_source.file =
> > > git_pathdup("config.worktree");
> > > + else if (worktrees[0] && worktrees[1])
> > > + die(_("--worktree cannot be used with multiple "
> > > + "working trees unless the config\n"
> > > + "extension worktreeConfig is enabled. "
> > > + "Please read \"CONFIGURATION FILE\"\n"
> > > + "section in \"git help worktree\" for
> > > details"));
> > > + else
> > > + given_config_source.file = git_pathdup("config");
> >
> > I'm not sure I understand the purpose of allowing --worktree when only
> > a single worktree is present and extensions.worktreeConfig is not set.
> > Can you talk about it a bit more?
>
> Unified API. If I write a script to do stuff and want it to work in
> multiple worktrees as well, I should not need to do "if single
> worktree, use "git config --local", if multiple use "git config
> --worktree"". By using "git config --worktree" I tell git "this config
> is per-worktree" and git should do the right thing, single worktree or
> not.
That's what I thought, but I still don't understand how that unified
API is going to help if the script writer happens to have multiple
worktrees but doesn't have extensions.worktreeConfig set, in which
case the command will error out. I don't see how that simplifies
things, but perhaps I'm missing something obvious.
Re: [PATCH v2 8/8] reflog expire: cover reflog from all worktrees
On Sat, Sep 29, 2018 at 3:11 PM Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
> @@ -72,6 +72,11 @@ Options for `expire`
> +--single-worktree::
> + By default when `--all` is specified, reflogs from all working
> + trees are processed. This option limits the processing to reflogs
> + from the current working tree only.
Bikeshedding: I wonder if this should be named "--this-worktree" or
"--this-worktree-only" or if it should somehow be orthogonal to --all
rather than modifying it. (Genuine questions. I don't have the
answers.)
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> @@ -577,10 +585,18 @@ static int cmd_reflog_expire(int argc, const char
> **argv, const char *prefix)
> if (do_all) {
> struct collect_reflog_cb collected;
> + struct worktree **worktrees, **p;
> int i;
>
> memset(, 0, sizeof(collected));
> - for_each_reflog(collect_reflog, );
> + worktrees = get_worktrees(0);
> + for (p = worktrees; *p; p++) {
> + if (!all_worktrees && !(*p)->is_current)
> + continue;
> + collected.wt = *p;
> + for_each_reflog(collect_reflog, );
> + }
> + free_worktrees(worktrees);
Should this have a test in the test suite?
Re: [PATCH v2 5/8] revision.c: better error reporting on ref from different worktrees
On Sat, Sep 29, 2018 at 3:11 PM Nguyễn Thái Ngọc Duy wrote:
> Make use of the new ref aliases to pass refs from another worktree
> around and access them from the current ref store instead. This does
> not change any functionality, but when a problem arises, we would like
> the reported messages to mention full ref aliases, like this:
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -487,6 +487,28 @@ int submodule_uses_worktrees(const char *path)
> +void strbuf_worktree_ref(const struct worktree *wt,
> +struct strbuf *sb,
> +const char *refname)
> +{
> + if (wt && !wt->is_current) {
> + if (is_main_worktree(wt))
> + strbuf_addstr(sb, "main/");
I think this needs to be "main-worktree/", doesn't it?
> + else
> + strbuf_addf(sb, "worktrees/%s/", wt->id);
> + }
> + strbuf_addstr(sb, refname);
> +}
> diff --git a/worktree.h b/worktree.h
> @@ -108,4 +108,18 @@ extern const char *worktree_git_path(const struct
> worktree *wt,
> +/*
> + * Return a refname suitable for access from the current ref
> + * store. The result may be destroyed at the next call.
s/may/will/
Re: [PATCH v2 3/8] refs: new ref types to make per-worktree refs visible to all worktrees
On Sat, Sep 29, 2018 at 3:10 PM Nguyễn Thái Ngọc Duy wrote: > The main worktree has to be treated specially because well.. it's Nit: s/well../well.../ > special from the beginning. So HEAD from the main worktree is > acccessible via the name "main-worktree/HEAD" instead of > "worktrees/main/HEAD" because "main" could be just another secondary > worktree. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > @@ -216,6 +217,18 @@ directly under GIT_DIR instead of inside GIT_DIR/refs. > There are one > +Refs that are per working tree can still be accessed from another > +working tree via two special paths main-worktree and worktrees. The s/paths/paths,/ > +former gives access to per-worktree refs of the main working tree, > +while the former to all linked working trees. s/former/latter/ > diff --git a/t/t1415-worktree-refs.sh b/t/t1415-worktree-refs.sh > @@ -30,4 +30,50 @@ test_expect_success 'refs/worktree are per-worktree' ' > +test_expect_success 'ambiguous main-worktree/HEAD' ' > + mkdir -p .git/refs/heads/main-worktree && > + test_when_finished rm .git/refs/heads/main-worktree/HEAD && > + cp .git/HEAD .git/refs/heads/main-worktree/HEAD && Better to use "rm -f" for cleanup in case this 'cp' fails for some reason. > + git rev-parse main-worktree/HEAD 2>warn >/dev/null && You could probably omit the /dev/null redirect. > + grep "main-worktree/HEAD.*ambiguous" warn > +' > + > +test_expect_success 'ambiguous worktrees/xx/HEAD' ' > + mkdir -p .git/refs/heads/worktrees/wt1 && > + test_when_finished rm .git/refs/heads/worktrees/wt1/HEAD && Ditto "rm -f". > + cp .git/HEAD .git/refs/heads/worktrees/wt1/HEAD && > + git rev-parse worktrees/wt1/HEAD 2>warn >/dev/null && Ditto /dev/null. > + grep "worktrees/wt1/HEAD.*ambiguous" warn > +'
Re: [PATCH v2 1/1] roll wt_status_state into wt_status and populate in the collect phase
On Sat, Sep 29, 2018 at 2:55 PM Stephen P. Smith wrote: > Status variables were initialized in the collect phase and some > variables were later freed in the print functions. > > A "struct wt_status" used to be sufficient for the output phase to > work. It was designed to be filled in the collect phase and consumed > in the output phase, but over time some fields were added and output > phase started filling the fields. > > A "struct wt_status_state" that was used in other codepaths turned out > to be useful in the "git status" output. This is not tied to "struct > wt_status", so filling in the collect phase was not consistently > followed. > > Move the status state structure variables into the status state > structure and populate them in the collect functions. > > Create a new funciton to free the buffers that were being freed in the s/funciton/function/ > print function. Call this new function in commit.c where both the > collect and print functions were being called. > > Signed-off-by: Stephen P. Smith
Re: [PATCH 1/1] roll wt_status_state into wt_status and populate in the collect phase
On Fri, Sep 28, 2018 at 9:55 AM Taylor Blau wrote: > On Thu, Sep 27, 2018 at 09:49:36PM -0700, Stephen P. Smith wrote: > > When updating the collect and print functions, it was found that > > status variables were initialized in the collect phase and some > > variables were later freed in the print functions. > > Nit: I think that in the past Eric Sunshine has recommended that I use > active voice in patches, but "it was found" is passive. > > I tried to find the message that I was thinking of, but couldn't, so > perhaps I'm inventing it myself ;-). > > I'm CC-ing Eric to check my judgement. You're probably thinking of "imperative mood" (and perhaps [1]), which this commit message already uses when it says "Move the..." and "Create a new function..." (in the couple paragraphs following the part you quoted). > > Move the status state structure variables into the status state > > structure and populate them in the collect functions. > > > > Create a new funciton to free the buffers that were being freed in the s/funciton/function/ > > print function. Call this new function in commit.c where both the > > collect and print functions were being called. [1]: https://public-inbox.org/git/capig+ctozduqsaxh+w4h85m7en72yo09asdx+1kstswqbnb...@mail.gmail.com/
Re: [PATCH v2 2/2] worktree: add per-worktree config files
On Sat, Sep 29, 2018 at 11:30 AM Nguyễn Thái Ngọc Duy wrote:
> A new repo extension is added, worktreeConfig. When it is present:
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -2,8 +2,9 @@ CONFIGURATION FILE
> The Git configuration file contains a number of variables that affect
> +the Git commands' behavior. The files `.git/config` and optionally
> +`config.worktree` (see `extensions.worktreeConfig` below) in each
> +repository is used to store the configuration for that repository, and
s/is used/are/used/
> `$HOME/.gitconfig` is used to store a per-user configuration as
> fallback values for the `.git/config` file. The file `/etc/gitconfig`
> can be used to store a system-wide default configuration.
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -204,6 +204,36 @@ working trees, it can be used to identify worktrees. For
> example if
> +CONFIGURATION FILE
> +--
> +In this mode, specific configuration stays in the path pointed by `git
> +rev-parse --git-path config.worktree`. You can add or update
> +configuration in this file with `git config --worktree`. Older Git
> +versions may will refuse to access repositories with this extension.
s/may will/will/
> diff --git a/Documentation/gitrepository-layout.txt
> b/Documentation/gitrepository-layout.txt
> @@ -275,6 +280,9 @@ worktrees//locked::
> +worktrees//config.worktree::
> + Working directory specific configuration file.
I wonder if this deserves a quick mention in
Documentation/git-worktree.txt since other worktree-related files,
such as "worktrees//locked", are mentioned there.
> diff --git a/builtin/config.c b/builtin/config.c
> @@ -645,7 +649,20 @@ int cmd_config(int argc, const char **argv, const char
> *prefix)
> + else if (use_worktree_config) {
> + struct worktree **worktrees = get_worktrees(0);
> + if (repository_format_worktree_config)
> + given_config_source.file =
> git_pathdup("config.worktree");
> + else if (worktrees[0] && worktrees[1])
> + die(_("--worktree cannot be used with multiple "
> + "working trees unless the config\n"
> + "extension worktreeConfig is enabled. "
> + "Please read \"CONFIGURATION FILE\"\n"
> + "section in \"git help worktree\" for
> details"));
> + else
> + given_config_source.file = git_pathdup("config");
I'm not sure I understand the purpose of allowing --worktree when only
a single worktree is present and extensions.worktreeConfig is not set.
Can you talk about it a bit more?
Re: [PATCH v2 1/2] t1300: extract and use test_cmp_config()
On Sat, Sep 29, 2018 at 11:30 AM Nguyễn Thái Ngọc Duy wrote:
> In many config-related tests it's common to check if a config variable
> has expected value and we want to print the differences when the test
> fails. Doing it the normal way is three lines of shell code. Let's add
> a function do to all this (and a little more).
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -747,6 +747,30 @@ test_cmp() {
> +# similar to test_cmp but $2 is a config key instead of actual value
> +# it can also accept -C to read from a different repo, e.g.
Minor: maybe say that "-C " changes to for the git-config invocation.
> +# test_cmp_config -C xyz foo core.bar
> +#
> +# is sort of equivalent of
> +#
> +# test "foo" = "$(git -C xyz core.bar)"
Should be: $(git -C xyz config core.bar)
> +test_cmp_config() {
> + if [ "$1" = "-C" ]
> + then
Style:
if test "$1" = "-C"
then
...
> + shift &&
> + GD="-C $1" &&
> + shift
> + else
> + GD=
> + fi &&
> + echo "$1" >expected &&
If $1 starts with a hyphen, 'echo' might try interpreting it as an
option. Use printf instead:
printf "%s\n" "$1" &&
> + shift &&
> + git $GD config "$@" >actual &&
> + test_cmp expected actual
Please choose names other than "actual" and "expected" since those are
likely to clobber files of the same name which the test has set up
itself. (Or, at the very least, document that this function clobbers
those files -- but using better filenames is preferable.)
> +}
Re: Triggering "BUG: wt-status.c:476: multiple renames on the same target? how?"
On Thu, Sep 27, 2018 at 3:20 AM Elijah Newren wrote: > Subject: [PATCH] commit: fix erroneous BUG, 'multiple renames on the same > target? how?' > [...] > Signed-off-by: Elijah Newren > --- > diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh > @@ -359,4 +359,27 @@ test_expect_success 'new line found before status > message in commit template' ' > +test_expect_success 'setup empty commit with unstaged rename and copy' ' > + test_create_repo unstaged_rename_and_copy && > + ( > + cd unstaged_rename_and_copy && > + > + echo content >orig && > + git add orig && > + test_commit orig && > + > + cp -a orig new_copy && Non-portable -a option. Not in POSIX[1]. [1]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html > + mv orig new_rename && > + git add -N new_copy new_rename > + ) > +'
Re: t7005-editor.sh failure
On Wed, Sep 26, 2018 at 5:53 AM Martin Ågren wrote:
> I came up with the following commit message. What do you think about it?
>
> t7005-editor: quote filename to fix whitespace-issue
>
> Commit 4362da078e (t7005-editor: get rid of the SPACES_IN_FILENAMES
> prereq, 2018-05-14) removed code for detecting whether spaces in
> filenames work. Since we rely on spaces throughout the test suite
> ("trash directory.t1234-foo"), testing whether we can use the filename
> "e space.sh" was redundant and unnecessary.
>
> In simplifying the code, though, the commit introduced a regression around
> how spaces are handled, not in the /name/ of the script, but /in/ the
> script itself. The editor-script created looks like this:
>
> echo space >$1
>
> We will try to execute something like
>
> echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG
>
> Most shells seem to be able to figure out that the filename doesn't end
> with "trash" but continues all the way to "COMMIT_EDITMSG", but at least
> one shell chokes on this.
>
> Make sure that the editor-script quotes "$1".
This description of the behavior is misleading (actually, actively
wrong). Shells are not somehow inferring that the space is part of the
redirect filename. The missing piece is that the following all behave
the same:
echo foo bar >cow
echo >cow foo bar
echo foo >cow bar
That is, they all create a file named "cow" with content "foo bar".
So, in your example:
echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG
what is actually happening is that it is creating a file named
"/foo/t/trash" with content "space
directory.t7005-editor/.git/COMMIT_EDITMSG".
As for the "ambiguous redirect" diagnostic, that seems to be Bash
trying to be helpful in reporting what is likely a programming error
(that is, forgetting to double-quote the expansion).
Re: [PATCH v4 1/1] commit-reach: properly peel tags and clear flags
On Mon, Sep 24, 2018 at 4:58 PM Derrick Stolee via GitGitGadget
wrote:
> diff --git a/commit-reach.c b/commit-reach.c
> @@ -544,20 +544,42 @@ int can_all_from_reach_with_flag(struct object_array
> *from,
> {
> + from_one = deref_tag(the_repository, from_one,
> +"a from object", 0);
> + if (!from_one || from_one->type != OBJ_COMMIT) {
> + /* no way to tell if this is reachable by
> +* looking at the ancestry chain alone, so
> +* leave a note to ourselves not to worry about
> +* this object anymore.
> +*/
Style nit:
/*
* Multi-line comment
* formatting.
*/
Re: [PATCH] worktree: add per-worktree config files
On Sun, Sep 23, 2018 at 1:04 PM Nguyễn Thái Ngọc Duy wrote:
> A new repo extension is added, worktreeConfig. When it is present:
>
> - Repository config reading by default includes $GIT_DIR/config _and_
>$GIT_DIR/config.worktree. "config" file remains shared in multiple
>worktree setup.
>
> - The special treatment for core.bare and core.worktree, to stay
>effective only in main worktree, is gone. These config files are
>supposed to be in config.worktree.
"These config _settings_ are supposed to be..."
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -2,8 +2,9 @@ CONFIGURATION FILE
> The Git configuration file contains a number of variables that affect
> -the Git commands' behavior. The `.git/config` file in each repository
> -is used to store the configuration for that repository, and
> +the Git commands' behavior. The files `.git/config` and optionally
> +`config.worktree` (see `extensions.worktreeConfig` below) are each
s/are/in/
> +repository is used to store the configuration for that repository, and
> `$HOME/.gitconfig` is used to store a per-user configuration as
> diff --git a/t/t2029-worktree-config.sh b/t/t2029-worktree-config.sh
> @@ -0,0 +1,82 @@
> +cmp_config() {
> + if [ "$1" = "-C" ]; then
Style:
if test "$1" = "-C"
then
...
> +}
> +
> +test_expect_success 'setup' '
> + test_commit start &&
> + git config --worktree per.worktree is-ok &&
Did you want:
cmp_config false extensions.worktreeConfig
at this point in the test or something? It's not clear what the
intention is here.
> + git worktree add wt1 &&
> + git worktree add wt2 &&
> + git config --worktree per.worktree is-ok &&
> + cmp_config true extensions.worktreeConfig
> +'
> +test_expect_success 'core.bare no longer for main only' '
> + git config core.bare true &&
> + cmp_config true core.bare &&
> + cmp_config -C wt1 true core.bare &&
> + cmp_config -C wt2 true core.bare &&
> + git config --unset core.bare
Should this be?
test_when_finished "git config --unset core.bare" &&
or perhaps test_config()?
> +'
Re: Import/Export as a fast way to purge files from Git?
On Sun, Sep 23, 2018 at 9:05 AM Lars Schneider wrote: > I recently had to purge files from large Git repos (many files, many commits). > The usual recommendation is to use `git filter-branch --index-filter` to purge > files. However, this is *very* slow for large repos (e.g. it takes 45min to > remove the `builtin` directory from git core). I realized that I can remove > files *way* faster by exporting the repo, removing the file references, > and then importing the repo (see Perl script below, it takes ~30sec to remove > the `builtin` directory from git core). Do you see any problem with this > approach? A couple comments: For purging files from a history, take a look at BFG[1] which bills itself as "a simpler, faster alternative to git-filter-branch for cleansing bad data out of your Git repository history". The approach of exporting to a fast-import stream, modifying the stream, and re-importing is quite reasonable. However, rather than re-inventing, take a look at reposurgeon[2], which allows you to do major surgery on fast-import streams. Not only can it purge files from a repository, but it can slice, dice, puree, and saute pretty much any attribute of a repository. [1]: https://rtyley.github.io/bfg-repo-cleaner/ [2]: http://www.catb.org/esr/reposurgeon/
Re: [PATCH 7/8] fsck: check HEAD and reflog from other worktrees
On Sat, Sep 22, 2018 at 2:05 PM Nguyễn Thái Ngọc Duy wrote: > fsck is a repo-wide operation and should check all references no > matter which worktree they are associated to. > > Reported-by: Jeff King > Helped-by: Elijah Newren > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh > @@ -101,6 +101,45 @@ test_expect_success 'HEAD link pointing at a funny > place' ' > +test_expect_success 'HEAD link pointing at a funny object (from different > wt)' ' > + test_when_finished "mv .git/SAVED_HEAD .git/HEAD" && > + test_when_finished "rm -rf .git/worktrees wt" && > + git worktree add wt && > + mv .git/HEAD .git/SAVED_HEAD && > + echo >.git/HEAD && Perhaps use $ZERO_OID here instead of hardcoded "0..." string to play friendly with brian's bc/hash-independent-tests (in 'next'). Same for following test. > + # avoid corrupt/broken HEAD from interfering with repo discovery > + test_must_fail git -C wt fsck 2>out && > + cat out && Unneeded 'cat'. Debugging cruft? Same for other tests. > + grep "main/HEAD: detached HEAD points" out This message doesn't get localized, so no need for test_i18ngrep(). Okay. > +'
Re: [PATCH 5/8] revision.c: better error reporting on ref from different worktrees
On Sat, Sep 22, 2018 at 2:05 PM Nguyễn Thái Ngọc Duy wrote:
> Make use of the new ref aliases to pass refs from another worktree
> around and access them from the current ref store instead. This does
> not change any functionality, but when a problem shows up, we would
> report something like
>From this description, I had a hard time grasping that the first
example output is the desired one. Not necessarily worth a re-roll,
but had it said instead something like this:
...but when a problem arises, we would like the reported
messages to mention full ref aliases, like this:
it would have been more obvious.
More below...
> fatal: bad object worktrees/ztemp/HEAD
> warning: reflog of 'main/HEAD' references pruned commits
>
> instead of
>
> fatal: bad object HEAD
> warning: reflog of 'HEAD' references pruned commits
>
> which does not really tell where the refs are from.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -487,6 +487,28 @@ int submodule_uses_worktrees(const char *path)
> +void strbuf_worktree_ref(const struct worktree *wt,
> +struct strbuf *sb,
> +const char *refname)
> +{
> + if (wt && !wt->is_current) {
> + if (is_main_worktree(wt))
> + strbuf_addstr(sb, "main/");
> + else
> + strbuf_addf(sb, "worktrees/%s/", wt->id);
> + }
> + strbuf_addstr(sb, refname);
> +}
Seeing this use worktree->id to construct "worktrees//"
makes me wonder how the user is going to know the ID of a worktree in
order to specify such refs in the first place. For example, how is the
user going to know the in "git rev-parse worktrees//HEAD"? I
don't think we print the worktree ID anywhere, do we? Certainly, "git
worktree list" doesn't show it, and "git worktree add" stopped showing
it as of 2c27002a0a (worktree: improve message when creating a new
worktree, 2018-04-24).
> diff --git a/worktree.h b/worktree.h
> @@ -108,4 +108,18 @@ extern const char *worktree_git_path(const struct
> worktree *wt,
> +/*
> + * Return a refname suitable for access from the current ref
> + * store. The result may be destroyed at the next call.
> + */
If you re-roll, perhaps: s/may/will/
Re: [PATCH 3/8] refs: new ref types to make per-worktree refs visible to all worktrees
On Sat, Sep 22, 2018 at 2:05 PM Nguyễn Thái Ngọc Duy wrote:
> [...]
> The main worktree has to be treated specially because well.. it's
> special from the beginning. So HEAD from the main worktree is
> acccessible via the name "main/HEAD" (we can't use
> "worktrees/main/HEAD" because "main" under "worktrees" is not
> reserved).
Bikeshedding: I wonder if this would be more intuitive if called
simply "/HEAD" rather than "main/HEAD".
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> diff --git a/refs.c b/refs.c
> @@ -641,12 +641,32 @@ static int is_pseudoref_syntax(const char *refname)
> +static int is_main_pseudoref_syntax(const char *refname)
> +{
> + return skip_prefix(refname, "main/", ) &&
> + is_pseudoref_syntax(refname);
> +}
> +
> +static int is_other_pseudoref_syntax(const char *refname)
> +{
> + if (!skip_prefix(refname, "worktrees/", ))
> + return 0;
> + refname = strchr(refname, '/');
> + if (!refname)
> + return 0;
> + return is_pseudoref_syntax(refname + 1);
> +}
If the input is "worktrees/refs/" (nothing following the trailing
'/'), then an empty string will be passed to is_pseudoref_syntax(),
which will return true. Does that result in correct behavior? (Same
question about "main/" being passed to is_main_pseudoref_syntax().)
Re: [PATCH 2/8] Add a place for (not) sharing stuff between worktrees
On Sat, Sep 22, 2018 at 2:05 PM Nguyễn Thái Ngọc Duy wrote: > When multiple worktrees are used, we need rules to determine if > something belongs to one worktree or all of them. Instead of keeping > adding rules when new stuff comes, have a generic rule: > [...] > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > diff --git a/t/t1415-worktree-refs.sh b/t/t1415-worktree-refs.sh > @@ -0,0 +1,36 @@ > +test_expect_success 'setup' ' > + test_commit initial && > + test_commit wt1 && > + test_commit wt2 && > + git worktree add wt1 wt1 && > + git worktree add wt2 wt2 && > + git checkout initial > +' > + > +test_expect_success 'add refs/local' ' > + git update-ref refs/local/foo HEAD && > + git -C wt1 update-ref refs/local/foo HEAD && > + git -C wt2 update-ref refs/local/foo HEAD > +' Not at all worth a re-roll, but the "add refs/local" test seems like just more setup, thus could be rolled into the "setup" test (unless it will be growing in some non-setup way in later patches).
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 2:47 PM Taylor Blau wrote:
> When in a repository containing one or more alternates, Git would
> sometimes like to list references from its alternates. For example, 'git
> receive-pack' list the objects pointed to by alternate references as
> special ".have" references.
> [...]
> Signed-off-by: Taylor Blau
> ---
> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> @@ -0,0 +1,54 @@
> +expect_haves () {
> + printf "%s .have\n" $(git rev-parse $@) >expect
> +}
> +
> +test_expect_success 'with core.alternateRefsCommand' '
> + [...]
> + expect_haves a c >expect &&
This is not great. Both the caller of expect_haves() and
expect_haves() itself redirect to a file named "expect". This works,
but only by accident.
Better would be to make expect_haves() simply a generator to stdout
and let the caller redirect to the file rather than hardcoding the
filename in the function itself (much as extract_haves() takes it its
input on stdin rather than hardcoding a filename). If you take this
approach, then you'd probably want to rename the function, as well;
perhaps call it emit_haves() or something.
> + printf "" | git receive-pack fork >actual &&
> + extract_haves actual.haves &&
> + test_cmp expect actual.haves
> +'
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 2:47 PM Taylor Blau wrote:
> When in a repository containing one or more alternates, Git would
> sometimes like to list references from its alternates. For example, 'git
> receive-pack' list the objects pointed to by alternate references as
> special ".have" references.
> [...]
> Signed-off-by: Taylor Blau
> ---
> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> @@ -0,0 +1,54 @@
> +expect_haves () {
> + printf "%s .have\n" $(git rev-parse $@) >expect
> +}
Magic quoting behavior only kicks in when $@ is itself quoted, so this
should be:
printf "%s .have\n" $(git rev-parse "$@") >expect
However, as it's unlikely that you need magic quoting in this case,
you might get by with plain $* (unquoted).
Re: [PATCH 3/3] transport.c: introduce core.alternateRefsPrefixes
On Thu, Sep 20, 2018 at 2:04 PM Taylor Blau wrote: > The recently-introduced "core.alternateRefsCommand" allows callers to > specify with high flexibility the tips that they wish to advertise from > alternates. This flexibility comes at the cost of some inconvenience > when the caller only wishes to limit the advertisement to one or more > prefixes. > [...] > Signed-off-by: Taylor Blau > --- > diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh > @@ -44,4 +44,15 @@ test_expect_success 'with core.alternateRefsCommand' ' > +test_expect_success 'with core.alternateRefsPrefixes' ' > + test_config -C fork core.alternateRefsPrefixes "refs/tags" && > + cat >expect <<-EOF && > + $(git rev-parse one) .have > + $(git rev-parse three) .have > + $(git rev-parse two) .have > + EOF It's probably a matter of taste as to which is more readable, but this entire "cat+ printf "" | git receive-pack fork | extract_haves >actual && > + test_cmp expect actual > +'
