Re: [PATCH v2] rebase: introduce --update-branches option
Hi Warren
On 08/09/2019 00:44, Warren He wrote:
Everyone in this thread, thanks for your support and encouragement.
[...]
It should not really imply `--interactive`, but `--rebase-merges`.
`imply_interactive` doesn't fully switch on `--interactive`, i.e., causing the
editor to open. It only selects the backend, which I think we're saying is the
right thing. I've dropped the `-i` from the test description.
And we don't really have to imply --rebase-merges, in case someone would prefer
to linearize things, which who knows? Running that non-rebase-merges command in
the example scenario from my original post should give something like this:
I think it would probably be less confusing to default to preserving
merges and having an option to turn that off - people are going to be
surprised if their history is linearized.
```
A - B (master)
\
F (feat-f)
\
E (feat-e)
\
H (my-dev)
```
So for now I haven't moved the implementation into `make_script_with_merges`.
> [...]>
[handling `fixup`/`squash` chains]
I've moved `todo_list_add_branch_updates` to run before
`todo_list_rearrange_squash`. The rearranging pulls fixups out, causing the
branch update to "fall" onto the items before, and reinserts them between a
commit and its branch update, casing them to be included in the updated branch.
which is my opinion of the right thing to do. I've added a test about this with
the following scenario:
```
A - B (master)
\
I - J - fixup! I (fixup-early)
\
K - fixup! J (fixup-late)
```
which results in the following todo list with `--autosquash`:
```
pick 9eadc32 I
fixup 265fa32 fixup! I
pick a0754fc J
fixup e7d1999 fixup! J
exec git branch -f fixup-early
pick c8bc4af K
```
That makes sense I think
I'd like to suggest [`test_cmp_rev`] instead
I've updated the test to use `test_cmp_rev`. It's not with your suggested
invocation though. We don't update the `C` tag. I've referred to the rebased `C`
with `test_cmp_rev linear-early HEAD^` and similar for the other checks.
That sounds right
* * *
And then there's the discussion about using `exec git branch -f`. To summarize
the issues collected from the entire thread:
1. the changes aren't atomically applied at the end of the rebase
2. it fails when the branch is checked out in a worktree
3. it clobbers the branch if anything else updates it during the rebase
4. the way we prepare the unprefixed branch doesn't work right some exotic cases
5. the reflog message it leaves is uninformative
For #4, I think we've lucked out actually. The `load_ref_decorations` routine we
use determines that a ref is `DECORATION_REF_LOCAL` under the condition
`starts_with(refname, "refs/heads/")` (log-tree.c:114, add_ref_decoration), so
`prettify_refname` will find the prefix and skip it. But that's an invariant
maintained by two pieces of code pretty far away from each other.
For #5, for the convenience of readers, the reflog entry it leaves looks like
this:
```
00873f2 feat-e@{0}: branch: Reset to HEAD
```
Not great.
I haven't made any changes to this yet, but I've thought about what I want. My
favorite so far is to add a new todo command that just does everything right. It
would make a temparary ref `refs/rewritten-heads/xxx` (or something), and update
`refs/heads/xxx` at the end.
I think that's the best way to do it. If we had a command like 'branch
' that creates a ref to remember the current HEAD and saves
the current branch head. Then at the end rebase can update the branches
to point to the saved commits if the branch is unchanged. If the rebase
is aborted then we don't end up with some branches updated and others not.
Side Note
I'd avoid creating another worktree local ref refs/rewritten-heads/.
Either store them under refs/rewritten/ or refs/worktree/
Best Wishes
Phillip
I agree that requiring a separate update-ref step at the end of the todo list is
unfriendly. Manually putting in some branch update commands and then realizing
that they weren't applied would be extremely frustrating. I don't see the option
of using existing tools as the easiest-to-use solution.
I'm reluctant to combine this with the existing `label` command. So far it
sounds like we generally want to be more willing to skip branch updates while
performing the rebase, with aforementioned scenarios where something else
updates the branch before we do, or if the branch becomes checked out in a
worktree. We don't want to mess up the structure of a `rebase -r` as a result of
skipping some branch updates. I think it would be conceptually simpler and
implementation-wise less tricky if we didn't combine it with the `label` and
`reset` system.
Warren He (1):
rebase: introduce --update-branches option
Documentation/git-rebase.txt | 9
builtin/rebase.c | 11 -
sequ
Re: [RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option
On Sun, Sep 08, 2019 at 10:01:33PM -0600, Eric Freese wrote: > On Sun, Sep 9, 2019 at 4:34 PM Junio C Hamano wrote: > > > I guess with "%(if)...%(then)...%(else)...%(end)" you might be able > > to do either one of --include/--exclude without supporting the > > other, e.g. "--include='%(if)%(symref)%(then)%(else)not a > > symref%(end)" would be usable as "I do not want to see symrefs" in a > > system that supports only "--include" without "--exclude". > > This has made me realize that I can get the behavior I need by using `%(if)`. > > Exclude symrefs: > "%(if)%(symref)%(then)%(else)%(refname)%(end)" > > Only symrefs: > "%(if)%(symref)%(then)%(refname)%(end)" > > However, this still prints an empty line for each ref that does not match the > condition. This can be cleaned up by piping through `grep .`, but what would > you think of adding a new optional flag to git-for-each-ref to prevent it from > printing empty expanded format strings? Yeah, I think that is a much nicer direction, in that it covers many more cases. I'm tempted to suggest it should even be the default, since the blank lines are mostly useless (by definition they carry no information except the single bit of "there was a ref that didn't have any output for your format"). My only reservation is that for-each-ref is plumbing, and it's _possible_ somebody is counting up the blank lines as part of some workflow (say, counting up tags and non-tags). It seems kind of unlikely, though. Much more likely is having an output format that has some non-blank elements and some blank, but the proposal wouldn't change the behavior there at all. -Peff
Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'
On 9/5/2019 2:57 PM, Junio C Hamano wrote: Jeff King writes: So these patches are punting on the greater question of why we want to parse so early, and are not making anything worse. AFAICT, "clone --filter=sparse:oid" has never worked (even though our tests did cover the underlying rev-list and pack-objects code paths). ... TBH, I'm not sure why the original is so eager to parse early. I guess it allows: - a dual use of the options parser; we can use it both to sanity-check the options before sending them to a server, and to actually use the filter ourselves. - earlier detection maybe gives us a cleaner error path (e.g., rev-list can do its own error handling). But I'd think doing it when we actually initialize the filter would be enough. I.e., if we want to go all the way, I think this two-patch series could basically be replaced with something like the (totally untested) approach below, which just pushes the parsing closer to the point-of-use. Adding Jeff Hostetler to the cc, in case he recalls any reason not to use that approach. Thanks. I think both of Peff's guesses are correct. IIRC I wrote the original parse_list_objects_filter() and friends to syntax check the command line arguments of rev-list. In hindsight, this looks a bit aggressive at that layer, or rather now that it is being used by various places in other commands (such as parsing messages from the wire), it shouldn't call die() as Peff suggests. I like the code Peff suggests. Making parse_list_objects_filter() a bit simpler and not call die(). Callers should then check the function return value as necessary. It would be nice if we could continue to let parse_list_objects_filter() do the syntax checking -- that is, we can still check that we received a ulong in "blob:limit:" and that "sparse:oid:" looks like a hex value, for example. Just save the actual lookup to the higher layer, if and when that makes sense. Jeff
Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions
[adding git list] On 9/5/19 7:25 AM, Christian Schoenebeck wrote: > How are you sending patches ? With git send-email ? If so, maybe you > can > pass something like --from='"Christian Schoenebeck" > '. Since this is a different string, git will > assume you're sending someone else's patch : it will automatically add > an > extra From: made out of the commit Author as recorded in the git tree. >>> >>> I think it is probably as simple as a 'git config' command to tell git >>> to always put a 'From:' in the body of self-authored patches when using >>> git format-patch; however, as I don't suffer from munged emails, I >>> haven't actually tested what that setting would be. > > Well, I tried that Eric. The expected solution would be enabling this git > setting: > > git config [--global] format.from true > https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom > > But as you can already read from the manual, the overall behaviour of git > regarding a separate "From:" line in the email body was intended solely for > the use case sender != author. So in practice (at least in my git version) > git > always makes a raw string comparison between sender (name and email) string > and author string and only adds the separate From: line to the body if they > differ. > > Hence also "git format-patch --from=" only works here if you use a different > author string (name and email) there, otherwise on a perfect string match it > is simply ignored and you end up with only one "From:" in the email header. git folks: How hard would it be to improve 'git format-patch'/'git send-email' to have an option to ALWAYS output a From: line in the body, even when the sender is the author, for the case of a mailing list that munges the mail headers due to DMARC/DKIM reasons? > > So eventually I added one extra character in my name for now and removed it > manually in the dumped emails subsequently (see today's > "[PATCH v7 0/3] 9p: Fix file ID collisions"). > > Besides that direct string comparison restriction; git also seems to have a > bug here. Because even if you have sender != author, then git falsely uses > author as sender of the cover letter, whereas the emails of the individual > patches are encoded correctly. At any rate, I'm glad that you have figured out a workaround, even if painful, while we wait for git to provide what we really need. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [PATCH v2] rebase: introduce --update-branches option
Hi,
On Mon, 9 Sep 2019, Phillip Wood wrote:
> On 08/09/2019 00:44, Warren He wrote:
> > Everyone in this thread, thanks for your support and encouragement.
> > [...]
> > > It should not really imply `--interactive`, but `--rebase-merges`.
> >
> > `imply_interactive` doesn't fully switch on `--interactive`, i.e., causing
> > the
> > editor to open. It only selects the backend, which I think we're saying is
> > the
> > right thing. I've dropped the `-i` from the test description.
> >
> > And we don't really have to imply --rebase-merges, in case someone would
> > prefer
> > to linearize things, which who knows? Running that non-rebase-merges command
> > in
> > the example scenario from my original post should give something like this:
>
> I think it would probably be less confusing to default to preserving merges
s/preserving/rebasing/?
> and having an option to turn that off - people are going to be surprised if
> their history is linearized.
I don't think it makes any sense to linearize the history while updating
branches, as the commits will be all jumbled up. Imagine this history:
- A - B - C - D -
\ /
E - F
Typically, it does not elicit any "bug" reports, but this can easily be
linearized to
- A' - B' - E' - C' - F' -
In my mind, it makes no sense to update any local branches that pointed
to C and F to point to C' and F', respectively.
> [...]
> > * * *
> >
> > And then there's the discussion about using `exec git branch -f`. To
> > summarize
> > the issues collected from the entire thread:
> >
> > 1. the changes aren't atomically applied at the end of the rebase
> > 2. it fails when the branch is checked out in a worktree
> > 3. it clobbers the branch if anything else updates it during the rebase
> > 4. the way we prepare the unprefixed branch doesn't work right some exotic
> > cases
> > 5. the reflog message it leaves is uninformative
> >
> > For #4, I think we've lucked out actually. The `load_ref_decorations`
> > routine we
> > use determines that a ref is `DECORATION_REF_LOCAL` under the condition
> > `starts_with(refname, "refs/heads/")` (log-tree.c:114, add_ref_decoration),
> > so
> > `prettify_refname` will find the prefix and skip it. But that's an invariant
> > maintained by two pieces of code pretty far away from each other.
> >
> > For #5, for the convenience of readers, the reflog entry it leaves looks
> > like this:
> >
> > ```
> > 00873f2 feat-e@{0}: branch: Reset to HEAD
> > ```
> >
> > Not great.
> >
> > I haven't made any changes to this yet, but I've thought about what I want.
> > My
> > favorite so far is to add a new todo command that just does everything
> > right. It
> > would make a temparary ref `refs/rewritten-heads/xxx` (or something), and
> > update
> > `refs/heads/xxx` at the end.
>
> I think that's the best way to do it. If we had a command like 'branch
> ' that creates a ref to remember the current HEAD and saves the
> current branch head. Then at the end rebase can update the branches to point
> to the saved commits if the branch is unchanged. If the rebase is aborted then
> we don't end up with some branches updated and others not.
I'd avoid cluttering the space with more commands. For `branch`, for
example, the natural short command would be `b`, but that already means
`break`.
In contrast, I would think that
label --update-branch my-side-track
would make for a nicer read than
label my-side-track
branch my-side-track
Of course, it would be a lot harder to bring back the safety of `git
update-ref`'s `` safe-guard, in either forms.
And of course, the first form would _require_ the logic to be moved to
`make_script_with_merges()` because we could not otherwise guarantee
that the labels corresponding to local branch names aren't already in
use, for different commits.
> Side Note
> I'd avoid creating another worktree local ref refs/rewritten-heads/.
> Either store them under refs/rewritten/ or refs/worktree/
Yep.
Ciao,
Dscho
Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions
On Mon, Sep 09, 2019 at 09:05:45AM -0500, Eric Blake wrote:
> > But as you can already read from the manual, the overall behaviour of git
> > regarding a separate "From:" line in the email body was intended solely for
> > the use case sender != author. So in practice (at least in my git version)
> > git
> > always makes a raw string comparison between sender (name and email) string
> > and author string and only adds the separate From: line to the body if they
> > differ.
> >
> > Hence also "git format-patch --from=" only works here if you use a
> > different
> > author string (name and email) there, otherwise on a perfect string match
> > it
> > is simply ignored and you end up with only one "From:" in the email header.
>
> git folks:
>
> How hard would it be to improve 'git format-patch'/'git send-email' to
> have an option to ALWAYS output a From: line in the body, even when the
> sender is the author, for the case of a mailing list that munges the
> mail headers due to DMARC/DKIM reasons?
It wouldn't be very hard to ask format-patch to just handle this
unconditionally. Something like:
diff --git a/pretty.c b/pretty.c
index e4ed14effe..9cf79d7874 100644
--- a/pretty.c
+++ b/pretty.c
@@ -451,7 +451,8 @@ void pp_user_info(struct pretty_print_context *pp,
map_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
if (cmit_fmt_is_mail(pp->fmt)) {
- if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {
+ if (pp->always_use_in_body_from ||
+ (pp->from_ident && ident_cmp(pp->from_ident, &ident))) {
struct strbuf buf = STRBUF_INIT;
strbuf_addstr(&buf, "From: ");
but most of the work would be ferrying that option from the command line
down to the pretty-print code.
That would work in conjunction with "--from" to avoid a duplicate. It
might require send-email learning about the option to avoid doing its
own in-body-from management. If you only care about send-email, it might
be easier to just add the option there.
-Peff
Re: [PATCH v2] grep: skip UTF8 checks explicitly
ping any feedback on code/approach highly appreciated Carlo On Wed, Aug 28, 2019 at 9:57 AM Carlo Arenas wrote: > > FWIW, the changes in grep.h are IMHO optional and hadn't been really > tested as I couldn't find a version of PCRE1 old enough to trigger it > but I am hoping are simple enough to work. > > As in my original proposal, there is no test because this is going to > (as documented) trigger undefined behaviour and so I don't see how we > can reliably test that. Goes without saying tthough, that the same is > triggered when JIT is enabled and the JIT fast path is being used, so > this is not a regression and will be more visible once > ab/pcre-jit-fixes gets merged because we are moving out of the JIT > fast path with a patch[0] in that series > > While Ævar made a point[1] that this wasn't a solution to the problem, > it was because PCRE2 could have a better one (still missing but based > on ab/pcre-jit-fixes), and it was expected that PCRE2 will be used a > lot more and so it wasn't a good idea for it (something that doesn't > apply to PCRE1) > > the patch was based on maint (like all bugfixes) but applies cleanly > to master and next, it will conflict with pu but for easy of merge I'd > applied it on top of cb/pcre1-cleanup and make it available in > GitHub[2]; that branch could be used as well as a reroll for that > topic (if that is preferred) > > the error message hasn't been changed on this patch, as it might make > sense to improve it as well for PCRE2, but at least shouldn't be > triggered anymore (ex, from running a freshly built git without the > patch and linked against a non JIT enabled PCRE1): > > $ ./git-grep -P 'Nguyễn Thái.Ngọc' > .mailmap:Nguyễn Thái Ngọc Duy > fatal: pcre_exec failed with error code -10 > > Carlo > > [0] 685668faaa (grep: stop using a custom JIT stack with PCRE v1, 2019-07-26) > [1] https://public-inbox.org/git/87lfwn70nb@evledraar.gmail.com/ > [2] https://github.com/carenas/git/tree/pcre1-cleanup
RE: O_NONBLOCK: should I blame git or ssh?
I think I've just figured out why my workaround works. When ssh sets O_NONBLOCK on stdout, it's setting it on the write side of the pipe that git created. Nothing else writes to that pipe, so this does not cause a problem. Presumably, ssh itself is able to deal with writing to a non-blocking "file description". When ssh sets O_NONBLOCK on stderr, it's setting it on the pipe created by Jenkins. But that's the same pipe that my workaround program does the select() on, so my workaround can compensate. When I run "make | workaround", the write side of *that* pipe is never made non-blocking, and that's the one that make itself writes to. Ok, I think all mysteries are solved, but I'm still not sure what the moral of the story is. This seems like it could lead to some really difficult to debug problems, but it's not clear to me what the right long-term fix should be. For now, we'll just leave our workaround in place and think about tweaking our makefile to redirect git's stderr.
Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits
Jeff King writes: > On Fri, Sep 06, 2019 at 09:59:04AM -0700, Junio C Hamano wrote: > >> Jeff King writes: >> >> > So far so good. But now imagine we call parse_commit_buffer() again, and >> > we re-parse. How does that interact with the half-parsed state? Some of >> > it works OK (e.g., lookup_tree() would find the same tree). Some not so >> > much (I think we'd keep appending parents at each call). >> > >> > I guess this might not be too bad to handle. Value fields like >> > timestamp_t are OK to overwrite. Pointers to objects likewise, since the >> > memory is owned elsewhere. If we see existing parent pointers in an >> > object we're parsing, we could probably free them under the assumption >> > they're leftover cruft. Likewise for the "tag" field of "struct tag", >> > which is owned by the struct and should be freed. >> >> Yeah, or clear them before returning with .corrupt bit set? > > This was my attempt to avoid dealing with a .corrupt bit. :) Then, clear them before returning with .parsed bit clear?
Re: [RFC PATCH 1/1] commit-graph.c: die on un-parseable commits
On Mon, Sep 09, 2019 at 09:39:41AM -0700, Junio C Hamano wrote: > Jeff King writes: > > > On Fri, Sep 06, 2019 at 09:59:04AM -0700, Junio C Hamano wrote: > > > >> Jeff King writes: > >> > >> > So far so good. But now imagine we call parse_commit_buffer() again, and > >> > we re-parse. How does that interact with the half-parsed state? Some of > >> > it works OK (e.g., lookup_tree() would find the same tree). Some not so > >> > much (I think we'd keep appending parents at each call). > >> > > >> > I guess this might not be too bad to handle. Value fields like > >> > timestamp_t are OK to overwrite. Pointers to objects likewise, since the > >> > memory is owned elsewhere. If we see existing parent pointers in an > >> > object we're parsing, we could probably free them under the assumption > >> > they're leftover cruft. Likewise for the "tag" field of "struct tag", > >> > which is owned by the struct and should be freed. > >> > >> Yeah, or clear them before returning with .corrupt bit set? > > > > This was my attempt to avoid dealing with a .corrupt bit. :) > > Then, clear them before returning with .parsed bit clear? But then somebody calling parse_commit() and getting an error return doesn't get to see the full extent of what we managed to parse. Which I thought was one of your original points: people should be able to get whatever information we can get out of even a corrupted or malformed object. If we keep that requirement, then we have to either clear them at the start of the re-parsing step, or we have to avoid re-parsing entirely by using an extra bit to say "parsed but had an error". I've sent messy "something like this" patches for both strategies. I think the latter is cleaner conceptually, but the former doesn't require giving up a flag bit. -Peff
Re: Git in Outreachy December 2019?
On Sun, Sep 08, 2019 at 08:26:10PM +0530, Pratyush Yadav wrote: > I'd like to put out a proposal regarding first contributions and micro > projects. > > I have a small list of small isolated features and bug fixes that > _I think_ git-gui would benefit with. And other people using it can > probably add their pet peeves and issues as well. My question is, are > these something new contributors should try to work on as an > introduction to the community? Since most of these features and fixes > are small and isolated, they should be pretty easy to work on. And I > think people generally find UI apps a little easier to work on. > > But I'll play the devil's advocate on my proposal and point out some > problems/flaws: > - Git-gui is written in Tcl, and git in C (and other languages too, but > not Tcl). That means while people do get a feel of the community and > general workflow, they don't necessarily get a feel of the actual git > internal codebase. > - Since I don't see a git-gui related project worth being into the > Outreachy program, it essentially means they will likely not work on > anything related to their project. > - Git-gui is essentially a wrapper on top of git, so people won't get > exposure to the git internals. > > I'd like to hear your and the rest of the community's thoughts about > this proposal, and whether it will be a good idea or not. Right, I came up with similar devil's advocate arguments. :) I'm not totally opposed, because part of the point of these microprojects just getting people familiar with interacting with the community and submitting a patch. They're not always in the same area the intern intends to work, just because there's not always a trivial problem to be solved there. So we do look at it mostly as a "can you do this basic test" test, and not necessarily as a prelude to the project. But it would be nice if it were at least in the same _language_ that the ultimate project will be done in. Because we're evaluating the applicant's ability to write code in that language, too. So I dunno. I am on the fence. -Peff
Re: O_NONBLOCK: should I blame git or ssh?
On Sun, Sep 08, 2019 at 02:18:15PM +, Douglas Graham wrote: > When I collected that strace output, I had stdout redirected to a pipe to my > workaround program, but I did not redirect stderr. So ssh made stdout > non-blocking, > but since stderr was still connected to my terminal, it didn't touch that. > But when > this build is run under Jenkins, both stdout and stderr are connected to a > pipe that > Jenkins creates to collect output from the build. I assume that when git runs > ssh, it > does not redirect ssh's stderr to its own pipe, it only redirects stdout. So > I think > ssh will be messing with both pipes when this build is run under Jenkins. OK, that makes sense. > Now that I have a fairly good understanding of what's happening, I think I > can work > around this occasional error by redirecting git's stderr to a file or > something like > that, but it's taken us a long time to figure this out, so I wonder if a more > permanent > fix shouldn't be implement, so others don't run into the same problem. A > google for > "make: write error" indicates that we're not the first to have this problem > with > parallel builds, although in the other cases I've found, a specific version > of the > Linux kernel was being blamed. Maybe that was a different problem. > > I guess git could workaround this by redirecting stderr, but the real problem > is probably > with ssh, although it's not clear to me what it should do differently. It > does some > somehow backwards to me that that it only makes a descriptor non-blocking if > it doesn't > refer to a TTY, but it does the same thing in at least three different places > so I guess > that's not a mistake. Where would git redirect the stderr to? We definitely want to make sure it goes to our original stderr, since it can have useful content for the user to see. We could make a new pipe and then pump the output back to our original stderr. But besides being complex, that fools the downstream programs about whether stderr is a tty (I don't know whether ssh cares, but certainly git itself uses that to decide on some elements of the output, mostly progress meters). So I think it would make more sense to talk to ssh folks about why this momentary O_NONBLOCK setting happens, and if it can be avoided. -Peff
Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'
On Mon, Sep 09, 2019 at 09:54:36AM -0400, Jeff Hostetler wrote: > It would be nice if we could continue to let parse_list_objects_filter() > do the syntax checking -- that is, we can still check that we received a > ulong in "blob:limit:" and that "sparse:oid:" looks like a hex > value, for example. Just save the actual lookup to the higher > layer, if and when that makes sense. Yeah, I agree that is the right place to do syntactic checking. But I think we can't do much checking for the sparse-oid. We currently accept not just a hex oid, but any name. And I think that is useful; I should be able to say "master:sparse-file" and have it resolved by the remote side. So it really is "any name which is syntactically resolvable as a sha1 expression". At which point I think you might as well punt and just wait until we _actually_ try to resolve the name to see if it's valid. I'll work up what I sent earlier into a real patch, and include some of this discussion. -Peff
Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'
Jeff Hostetler writes: > It would be nice if we could continue to let parse_list_objects_filter() > do the syntax checking -- that is, we can still check that we received a > ulong in "blob:limit:" and that "sparse:oid:" looks like a hex > value, for example. Just save the actual lookup to the higher > layer, if and when that makes sense. Hmmm, am I misremembering things or did I hear somebody mention in this thread that people give "sparse:oid:master" (not blob object name in hex, but a refname) and expect the other side to resolve it?
Re: [PATCH 1/2] Quit passing 'now' to date code
On Sun, Sep 08, 2019 at 06:47:10PM -0700, Stephen P. Smith wrote:
> As part of a previous patch set, the get_time() function was added to
> date.c eliminating the need to pass a `now` parameter from the test
> code.
I'm glad to see this cleanup. I think it is worth explaining a bit more,
though, why this hunk in particular:
> @@ -103,22 +103,14 @@ static void getnanos(const char **argv)
>
> int cmd__date(int argc, const char **argv)
> {
> - struct timeval now;
> const char *x;
> -
> x = getenv("GIT_TEST_DATE_NOW");
> - if (x) {
> - now.tv_sec = atoi(x);
> - now.tv_usec = 0;
> - }
> - else
> - gettimeofday(&now, NULL);
...is doing the right thing, since it was the site that actually used
the parameters that are being deleted. Maybe something like:
Commit b841d4ff43 (Add `human` format to test-tool, 2019-01-28) added
a get_time() function which allows $GIT_TEST_DATE_NOW in the
environment to override the current time. So we no longer need to
interpret that variable in cmd__date().
Likewise, we can stop passing the "now" parameter down through the
date functions, since nobody uses them. Note that we do need to make
sure all of the previous callers that took a "now" parameter are
correctly using get_time().
which I think explains all of the hunks.
-Peff
Re: [PATCH 2/2] test_date.c: Remove reference to GIT_TEST_DATE_NOW
On Sun, Sep 08, 2019 at 06:47:11PM -0700, Stephen P. Smith wrote: > Remove the reference to the GIT_TEST_DATE_NOW which is done in date.c. > The intialization of variable x with the value from GIT_TEST_DATE_NOW > is unneeded since x is initalized by skip_prefix(). It took me a minute to understand what this second sentence meant. I'd have actually expected "x" to go away, looking at the diff context. Maybe a more clear explanation would be: We can't get rid of the "x" variable, since it serves as a generic scratch variable for parsing later in the function. (I'd also probably have just rolled this into patch 1, but I'm OK with it either way). -Peff
Re: [PATCH] Documentation: fix build with Asciidoctor 2
"brian m. carlson" writes: > There are a couple ways around this. > > 1. We can force xmlto to use the DocBook 5 stylesheets with the "-x" > flag, but we have to know where they are. Debian and Fedora have them > in different places, so we'd need a knob to figure out where they are. > > 2. We can force xmlto to use a custom stylesheet with "-x" that merely > imports the DocBook 5 stylesheets using a URL. If the user has the > DocBook 5 stylesheets installed and XML catalogs configured (the default > on Linux distributions), then everything will just work and the system > will resolve it to the local copy. If, however, things are not properly > configured, this will result in multiple network downloads for each > manual page. > > 3. We can give up on xmlto and do things ourselves. This has the same > problem as option 1, since we need to learn how to find the stylesheets. > > 4. We can send a patch to xmlto to make it use the proper stylesheets > for DocBook 5 and hope upstream does a release and everyone upgrades > shortly. Since xmlto is not at all active upstream, this seems like it > may be an imprudent choice. > > 5. We can send a patch to the DocBook stylesheets and have them include > both the namespaced and unnamespaced versions of the element names in > both sets of stylesheets and hope everyone upgrades. > > My personal preference is #2; I think that seems like the best choice > forward. XML catalogs are well understood and well configured on Linux > distributions. Homebrew supports them adequately, but you have to add > an environment variable to your shell configuration to enable them. Of > course, if you're doing _anything_ with XML, you'll have them enabled. Sounds sensible and well reasoned.
Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting
Jeff King writes: > Generation numbers are a little trickier, though, because they imply an > actual topological traversal. It might actually be easier to couple this > with the connectivity check we do after index-pack finishes (though I've > often wondered if we could drop that check in favor of making index-pack > smarter about finding the boundaries). Currently after scanning the incoming objects with the fsck machinery, we count the number of objects that are pointed at by these objects in the pack and are not in the pack in the builtin/index-pack.c::check_object() function; at that point, we no longer know who points at the object in question, which is needed if we want to compute the boundary, so we need a bit of work here. With boundary information, we could be smarter about lazy fetching, I presume?
Re: [PATCH 3/3] commit-graph.c: handle corrupt/missing trees
Jeff King writes: >> Answer. There is a single hit inside fsck.c that wants to report >> an error without killing ourselves in fsck_commit_buffer(). I >> however doubt its use of get_commit_tree() is correct in the >> first place. The function is about validating the commit object >> payload manually, without trusting the result of parse_commit(), >> and it does read the object name of the tree object; the call to >> get_commit_tree() used for reporting the error there should >> probably become has_object() on the tree_oid. > > I actually think that check should be removed entirely. That function is > about checking the syntactic validity of the object itself, not about > connectivity (which is handled separately). We already check that we > have a valid "tree" pointer earlier in the function. Of course, you're right.
Re: [PATCH v4 0/6] rebase -i: support more options
Rohit Ashiwal writes: > Following the suggestion of Phillip I've rebased my patch on master > (745f681289) > and cherry-picking b0a3186140. Sorry, but that's horrible. The latter does not even cleanly apply on the former. Let me see if I can find time to whip this into a reasonable shape. Thanks.
Re: [RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option
Eric Freese writes: > However, this still prints an empty line for each ref that does not match the > condition. This can be cleaned up by piping through `grep .`, but what would > you think of adding a new optional flag to git-for-each-ref to prevent it from > printing empty expanded format strings? Offhand I do not think of a reason why anybody wants to give a format that results in a total blank, so it might not even need to be an optional behaviour, but certainly a new option that triggers such a behaviour would be a safe thing to add.
Re: [PATCH v2] rebase: introduce --update-branches option
Johannes Schindelin writes: > In contrast, I would think that > > label --update-branch my-side-track > > would make for a nicer read than > > label my-side-track > branch my-side-track Because labelling while recreating a mergey history is conceptually the same as temporarily updating the branch head from end users' point of view, so this sounds quite sensible.
Re: [RFC 04/11] coccicheck: detect hashmap_entry.hash assignment
Eric Wong writes:
> Eric Wong wrote:
>> By renaming the "hash" field to "_hash", it's easy to spot
>> improper initialization of hashmap_entry structs which
>> can leave "hashmap_entry.next" uninitialized.
>
> Junio, I'm planning to reroll this series.
> (Sorry for not following up sooner)
>
> Would you prefer I drop 04/11 "hashmap_entry: detect improper initialization"
> in favor of the following? Thanks.
Automation is good ;-)
FWIW, I do not mind the original 04/11 myself too much, though.
Thanks.
> -8<
> Subject: [PATCH 4/11] coccicheck: detect hashmap_entry.hash assignment
>
> Assigning hashmap_entry.hash manually leaves hashmap_entry.next
> uninitialized, which can be dangerous once the hashmap_entry is
> inserted into a hashmap. Detect those assignments and use
> hashmap_entry_init, instead.
>
> Signed-off-by: Eric Wong
> ---
> contrib/coccinelle/hashmap.cocci | 16
> 1 file changed, 16 insertions(+)
> create mode 100644 contrib/coccinelle/hashmap.cocci
>
> diff --git a/contrib/coccinelle/hashmap.cocci
> b/contrib/coccinelle/hashmap.cocci
> new file mode 100644
> index 00..d69e120ccf
> --- /dev/null
> +++ b/contrib/coccinelle/hashmap.cocci
> @@ -0,0 +1,16 @@
> +@ hashmap_entry_init_usage @
> +expression E;
> +struct hashmap_entry HME;
> +@@
> +- HME.hash = E;
> ++ hashmap_entry_init(&HME, E);
> +
> +@@
> +identifier f !~ "^hashmap_entry_init$";
> +expression E;
> +struct hashmap_entry *HMEP;
> +@@
> + f(...) {<...
> +- HMEP->hash = E;
> ++ hashmap_entry_init(HMEP, E);
> + ...>}
Re: [PATCH 1/2] log: test --decorate-refs-exclude with --simplify-by-decoration
René Scharfe writes: > Demonstrate that a decoration filter given with --decorate-refs-exclude > is inadvertently overruled by --simplify-by-decoration. > > Reported-by: Étienne SERVAIS > Signed-off-by: René Scharfe > --- > t/t4202-log.sh | 15 +++ > 1 file changed, 15 insertions(+) Looks vaguely familiar ;-) Thanks for resurrecting it.
Re: [PATCH 0/2] Date test code clean-up
"Stephen P. Smith" writes: > As part of a previous patch submission[1], a cleanup patch was > suggested to remove a now unnecessary passing of a date environment > variable to the production code. It looks like that the idea to realize that get_time() that is aware of GIT_TEST_DATE_NOW is always called before functions like show_date_relative(), approxidate_str() and approxidate_careful(), and arrange it to be called in the lower level of the callchain, which makes sense to me. Thanks for tying the loose end.
Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions
Eric Blake writes: > How hard would it be to improve 'git format-patch'/'git send-email' to > have an option to ALWAYS output a From: line in the body, even when the > sender is the author, for the case of a mailing list that munges the > mail headers due to DMARC/DKIM reasons? I'd say that it shouldn't be so hard to implement than realizing what ahd why it is needed, designing what the end-user interaction would be (i.e. command line options? configuration variables? should it be per send-email destination?) and stating all of the above clearly in the documentation and the proposed commit log message. The reason you are asking is...? Am I smelling a volunteer?
Re: [PATCH v2] grep: skip UTF8 checks explicitly
Carlo Arenas writes: > ping > > any feedback on code/approach highly appreciated I'd prefer to see others weigh in before starting to merge the pcre stuff to 'next'. I do not mind taking this updated one that limits the scope to pcre1 as a pure regressino fix, if others agree. Thanks for keeping a tab on these pcre issues. >> While Ævar made a point[1] that this wasn't a solution to the problem, >> it was because PCRE2 could have a better one (still missing but based >> on ab/pcre-jit-fixes), and it was expected that PCRE2 will be used a >> lot more and so it wasn't a good idea for it (something that doesn't >> apply to PCRE1) >> >> the patch was based on maint (like all bugfixes) but applies cleanly >> to master and next, it will conflict with pu but for easy of merge I'd >> applied it on top of cb/pcre1-cleanup and make it available in >> GitHub[2]; that branch could be used as well as a reroll for that >> topic (if that is preferred) >> >> the error message hasn't been changed on this patch, as it might make >> sense to improve it as well for PCRE2, but at least shouldn't be >> triggered anymore (ex, from running a freshly built git without the >> patch and linked against a non JIT enabled PCRE1): >> >> $ ./git-grep -P 'Nguyễn Thái.Ngọc' >> .mailmap:Nguyễn Thái Ngọc Duy >> fatal: pcre_exec failed with error code -10 >> >> Carlo >> >> [0] 685668faaa (grep: stop using a custom JIT stack with PCRE v1, 2019-07-26) >> [1] https://public-inbox.org/git/87lfwn70nb@evledraar.gmail.com/ >> [2] https://github.com/carenas/git/tree/pcre1-cleanup
Re: [PATCH v4 0/6] rebase -i: support more options
On 09/09/2019 19:02, Junio C Hamano wrote: Rohit Ashiwal writes: Following the suggestion of Phillip I've rebased my patch on master (745f681289) and cherry-picking b0a3186140. Sorry, but that's horrible. The latter does not even cleanly apply on the former. Yes I had assumed that the cherry pick would become the first patch of this series and be dropped from pw/rebase-i-show-HEAD-to-reword. I should have been more explicit about that. Let me see if I can find time to whip this into a reasonable shape. As pw/rebase-i-show-HEAD-to-reword is slated for next perhaps these could build on that. The first patch needs am -3 to apply to that branch but the result looks ok and the rest apply as is. Best Wishes Phillip Thanks.
Re: What's cooking in git.git (Sep 2019, #01; Sat, 7)
Hi Junio, On Sat, Sep 07, 2019 at 10:26:53AM -0700, Junio C Hamano wrote: > * dl/complete-cherry-pick-revert-skip (2019-08-27) 3 commits > - status: mention --skip for revert and cherry-pick > - completion: add --skip for cherry-pick and revert > - completion: merge options for cherry-pick and revert > > The command line completion support (in contrib/) learned about the > "--skip" option of "git revert" and "git cherry-pick". > > Will merge to 'next'. Did we end up deciding whether or not we were going to drop "status: mention --skip for revert and cherry-pick"? > > * dl/use-sq-from-test-lib (2019-09-06) 1 commit > - t: use common $SQ variable > > Code cleanup. > > Will merge to 'next'. This needs a tiny update to its commit message: we should change `git grep =\'` to `git grep =\' t/`. > * dl/format-patch-cover-letter-subject (2019-09-05) 1 commit > - format-patch: learn --infer-cover-subject option > (this branch uses dl/format-patch-doc-test-cleanup.) > > "git format-patch --cover-letter" learned to optionally use the > first paragraph (typically a single-liner) of branch.*.description > as the subject of the cover letter. > > Reroll with a redesign with less emphasis on "subject" coming? Correct, I'll wait for the format-patch cleanup stuff to settle down before sending the reroll in. > * dl/remote-save-to-push (2018-12-11) 1 commit > - remote: add --save-to-push option to git remote set-url > > "git remote set-url" learned a new option that moves existing value > of the URL field to pushURL field of the remote before replacing > the URL field with a new value. > > Anybody who wants to champion this topic? > I am personally not yet quite convinced if this is worth pursuing. Perhaps it's time to drop this topic? Thanks, Denton
Re: [PATCH 2/2] compat/*.[ch]: remove extern from function declarations using spatch
Hi Denton,
On Wed, 4 Sep 2019, Denton Liu wrote:
> On Wed, Sep 04, 2019 at 11:43:06PM +0200, Johannes Schindelin wrote:
> >
> > On Wed, 4 Sep 2019, Denton Liu wrote:
> >
> > > In 554544276a (*.[ch]: remove extern from function declarations using
> > > spatch, 2019-04-29), we removed externs from function declarations using
> > > spatch but we intentionally excluded files under compat/ since some are
> > > directly copied from an upstream and we should avoid churning them so
> > > that manually merging future updates will be simpler.
> > >
> > > In the last commit, we determined the files which taken from an upstream
> > > so we can exclude them and run spatch on the remainder.
> > >
> > > This was the Coccinelle patch used:
> > >
> > > @@
> > > type T;
> > > identifier f;
> > > @@
> > > - extern
> > > T f(...);
> > >
> > > and it was run with:
> > >
> > > $ git ls-files compat/\*\*.{c,h} |
> > > xargs spatch --sp-file contrib/coccinelle/noextern.cocci
> > > --in-place
> > > $ git checkout -- \
> > > compat/regex/ \
> > > compat/inet_ntop.c \
> > > compat/inet_pton.c \
> > > compat/nedmalloc/ \
> > > compat/obstack.{c,h} \
> > > compat/poll/
> > >
> > > Coccinelle has some trouble dealing with `__attribute__` and varargs so
> > > we ran the following to ensure that no remaining changes were left
> > > behind:
> > >
> > > $ git ls-files compat/\*\*.{c,h} |
> > > xargs sed -i'' -e 's/^\(\s*\)extern \([^(]*([^*]\)/\1\2/'
> > > $ git checkout -- \
> > > compat/regex/ \
> > > compat/inet_ntop.c \
> > > compat/inet_pton.c \
> > > compat/nedmalloc/ \
> > > compat/obstack.{c,h} \
> > > compat/poll/
> >
> > I wonder whether we want to make this part of the (slightly misnamed)
> > "Static Analysis" job in our CI.
>
> Do you mean running cocci on all of our source files as opposed to just
> the files we compile? These two patches are part of an experimental (and
> unsubmitted) patchset that does exactly that. Seeing that there's
> interest, I'll try to send it in soon.
I look forward to it!
Dscho
[PATCH v2] cache-tree: do not lazy-fetch merge tree
When cherry-picking (for example), new trees may be constructed. During
this process, Git constructs the new tree in a struct strbuf, computes
the OID of the new tree, and checks if the new OID already exists on
disk. However, in a partial clone, the disk check causes a lazy fetch to
occur, which is both unnecessary (because we have the tree in the struct
strbuf) and likely to fail (because the remote probably doesn't have
this tree).
Do not lazy fetch in this situation.
Signed-off-by: Jonathan Tan
---
As requested in What's Cooking [1], here's a patch with an updated
commit message. Otherwise, the patch is exactly the same.
[1] https://public-inbox.org/git/xmqqd0gcm2zm@gitster-ct.c.googlers.com/
---
cache-tree.c | 2 +-
t/t0410-partial-clone.sh | 14 ++
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/cache-tree.c b/cache-tree.c
index c22161f987..9e596893bc 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -407,7 +407,7 @@ static int update_one(struct cache_tree *it,
if (repair) {
struct object_id oid;
hash_object_file(buffer.buf, buffer.len, tree_type, &oid);
- if (has_object_file(&oid))
+ if (has_object_file_with_flags(&oid,
OBJECT_INFO_SKIP_FETCH_OBJECT))
oidcpy(&it->oid, &oid);
else
to_invalidate = 1;
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 6415063980..3e434b6a81 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -492,6 +492,20 @@ test_expect_success 'gc stops traversal when a missing but
promised object is re
! grep "$TREE_HASH" out
'
+test_expect_success 'do not fetch when checking existence of tree we construct
ourselves' '
+ rm -rf repo &&
+ test_create_repo repo &&
+ test_commit -C repo base &&
+ test_commit -C repo side1 &&
+ git -C repo checkout base &&
+ test_commit -C repo side2 &&
+
+ git -C repo config core.repositoryformatversion 1 &&
+ git -C repo config extensions.partialclone "arbitrary string" &&
+
+ git -C repo cherry-pick side1
+'
+
. "$TEST_DIRECTORY"/lib-httpd.sh
start_httpd
--
2.23.0.162.g0b9fbb3734-goog
Re: [PATCH v4 0/6] rebase -i: support more options
Phillip Wood writes: > Yes I had assumed that the cherry pick would become the first patch of > this series and be dropped from pw/rebase-i-show-HEAD-to-reword. Ah, such an arrangement would have made a log more sense, indeed. > As pw/rebase-i-show-HEAD-to-reword is slated for next perhaps these > could build on that. The first patch needs am -3 to apply to that > branch but the result looks ok and the rest apply as is. As this is more or less "new feature" series, I do not mind too much to make it depend on another topic in flight that is getting solid. Thanks for helping.
Re: [PATCH 2/2] test_date.c: Remove reference to GIT_TEST_DATE_NOW
Jeff King writes: > On Sun, Sep 08, 2019 at 06:47:11PM -0700, Stephen P. Smith wrote: > >> Remove the reference to the GIT_TEST_DATE_NOW which is done in date.c. >> The intialization of variable x with the value from GIT_TEST_DATE_NOW >> is unneeded since x is initalized by skip_prefix(). > > It took me a minute to understand what this second sentence meant. I'd > have actually expected "x" to go away, looking at the diff context. > > Maybe a more clear explanation would be: We can't get rid of the "x" > variable, since it serves as a generic scratch variable for parsing > later in the function. > > (I'd also probably have just rolled this into patch 1, but I'm OK with > it either way). Thanks for saying everything ;-) I have nothing to add.
Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'
On 9/9/2019 1:12 PM, Junio C Hamano wrote: Jeff Hostetler writes: It would be nice if we could continue to let parse_list_objects_filter() do the syntax checking -- that is, we can still check that we received a ulong in "blob:limit:" and that "sparse:oid:" looks like a hex value, for example. Just save the actual lookup to the higher layer, if and when that makes sense. Hmmm, am I misremembering things or did I hear somebody mention in this thread that people give "sparse:oid:master" (not blob object name in hex, but a refname) and expect the other side to resolve it? You're right. I misspoke. I was thinking about the hex OID case and forgetting about the : form. Jeff
Re: [PATCH v2] cache-tree: do not lazy-fetch merge tree
Jonathan Tan writes: > When cherry-picking (for example), new trees may be constructed. During > this process, Git constructs the new tree in a struct strbuf, computes > the OID of the new tree, and checks if the new OID already exists on > disk. However, in a partial clone, the disk check causes a lazy fetch to > occur, which is both unnecessary (because we have the tree in the struct > strbuf) and likely to fail (because the remote probably doesn't have > this tree). I somehow smell that the above misses the point of the check in the first place, though. The reason why we are computing the tree object's name and seeing if we have it locally on disk is to decide if we want to record it in the cache tree, *without* writing the tree out to our object store, no? It is not just unnecessary but also against the goal of the codepath to lazily download it, even if the tree is available remotely. And it is irrelevant that there are cases the remote does not have it---we have no need to mention that we must be prepared to see the lazy fetch to fail. Even when they do have one, we do not want to fetch it and write to our object store. Isn't that what is going on? I thought I dug up the original that introduced the has_object_file() call to this codepath to make sure we understand why we make the check (and I expected the person who is proposing this change to do the same and record the finding in the proposed log message). I am running out of time today, and will revisit later this week (I'll be down for at least two days starting tomorrow, by the way). Thanks.
Re: What's cooking in git.git (Sep 2019, #01; Sat, 7)
Denton Liu writes: > Hi Junio, > > On Sat, Sep 07, 2019 at 10:26:53AM -0700, Junio C Hamano wrote: >> * dl/complete-cherry-pick-revert-skip (2019-08-27) 3 commits >> - status: mention --skip for revert and cherry-pick >> - completion: add --skip for cherry-pick and revert >> - completion: merge options for cherry-pick and revert >> >> The command line completion support (in contrib/) learned about the >> "--skip" option of "git revert" and "git cherry-pick". >> >> Will merge to 'next'. > > Did we end up deciding whether or not we were going to drop "status: > mention --skip for revert and cherry-pick"? If you are not convinced it is a good idea, we can easily drop it (and I do not mind dropping it---I am not convinced it is a good idea myself). >> * dl/remote-save-to-push (2018-12-11) 1 commit >> - remote: add --save-to-push option to git remote set-url >> >> "git remote set-url" learned a new option that moves existing value >> of the URL field to pushURL field of the remote before replacing >> the URL field with a new value. >> >> Anybody who wants to champion this topic? >> I am personally not yet quite convinced if this is worth pursuing. > > Perhaps it's time to drop this topic? OK. Thanks.
Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'
On 9/9/2019 1:08 PM, Jeff King wrote: On Mon, Sep 09, 2019 at 09:54:36AM -0400, Jeff Hostetler wrote: It would be nice if we could continue to let parse_list_objects_filter() do the syntax checking -- that is, we can still check that we received a ulong in "blob:limit:" and that "sparse:oid:" looks like a hex value, for example. Just save the actual lookup to the higher layer, if and when that makes sense. Yeah, I agree that is the right place to do syntactic checking. But I think we can't do much checking for the sparse-oid. We currently accept not just a hex oid, but any name. And I think that is useful; I should be able to say "master:sparse-file" and have it resolved by the remote side. Right. I forgot about the "master:sparse-file" case and was only thinking about the hex oid case. The sparse-file case is very useful. So it really is "any name which is syntactically resolvable as a sha1 expression". At which point I think you might as well punt and just wait until we _actually_ try to resolve the name to see if it's valid. I'll work up what I sent earlier into a real patch, and include some of this discussion. -Peff thanks Jeff
Re: [PATCH v2 17/16] t3427: accelerate this test by using fast-export and fast-import
Hi Elijah,
On Wed, 4 Sep 2019, Elijah Newren wrote:
> fast-export and fast-import can easily handle the simple rewrite that
> was being done by filter-branch, and should be faster on systems with a
> slow fork. Measuring the overall time taken for all of t3427 (not just
> the difference between filter-branch and fast-export/fast-import) shows
> a speedup of about 5% on Linux and 11% on Mac.
>
> Signed-off-by: Elijah Newren
> ---
> This patch is meant to be added onto the end of js/rebase-r-strategy; an
> earlier version of this patch conflicted js/rebase-r-strategy so now I'm
> basing on top of that series. The speedup is also less impressive now
> that there is only one filter-branch invocation being replaced instead of
> a handful. Still a nice speedup, though.
ACK!
Thanks,
Dscho
>
> t/t3427-rebase-subtree.sh | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
> index 39e348de16..bec48e6a1f 100755
> --- a/t/t3427-rebase-subtree.sh
> +++ b/t/t3427-rebase-subtree.sh
> @@ -59,7 +59,10 @@ test_expect_success 'setup' '
> test_commit files_subtree/master5 &&
>
> git checkout -b to-rebase &&
> - git filter-branch --prune-empty -f --subdirectory-filter files_subtree
> &&
> + git fast-export --no-data HEAD -- files_subtree/ |
> + sed -e "s%\([0-9a-f]\{40\} \)files_subtree/%\1%" |
> + git fast-import --force --quiet &&
> + git reset --hard &&
> git commit -m "Empty commit" --allow-empty
> '
>
> --
> 2.22.0.19.ga495766805
>
>
Re: [PATCH v2] cache-tree: do not lazy-fetch merge tree
Junio C Hamano writes: > Isn't that what is going on? I thought I dug up the original that > introduced the has_object_file() call to this codepath to make sure > we understand why we make the check (and I expected the person who > is proposing this change to do the same and record the finding in > the proposed log message). > > I am running out of time today, and will revisit later this week > (I'll be down for at least two days starting tomorrow, by the way). Here is what I came up with. The cache-tree datastructure is used to speed up the comparison between the HEAD and the index, and when the index is updated by a cherry-pick (for example), a tree object that would represent the paths in the index in a directory is constructed in-core, to see if such a tree object exists already in the object store. When the lazy-fetch mechanism was introduced, we converted this "does the tree exist?" check into an "if it does not, and if we lazily cloned, see if the remote has it" call by mistake. Since the whole point of this check is to repair the cache-tree by recording an already existing tree object opportunistically, we shouldn't even try to fetch one from the remote. Pass the OBJECT_INFO_SKIP_FETCH_OBJECT flag to make sure we only check for existence in the local object store without triggering the lazy fetch mechanism.
Re: [PATCH v2 17/16] t3427: accelerate this test by using fast-export and fast-import
Johannes Schindelin writes:
> Hi Elijah,
>
> On Wed, 4 Sep 2019, Elijah Newren wrote:
>
>> fast-export and fast-import can easily handle the simple rewrite that
>> was being done by filter-branch, and should be faster on systems with a
>> slow fork. Measuring the overall time taken for all of t3427 (not just
>> the difference between filter-branch and fast-export/fast-import) shows
>> a speedup of about 5% on Linux and 11% on Mac.
>>
>> Signed-off-by: Elijah Newren
>> ---
>> This patch is meant to be added onto the end of js/rebase-r-strategy; an
>> earlier version of this patch conflicted js/rebase-r-strategy so now I'm
>> basing on top of that series. The speedup is also less impressive now
>> that there is only one filter-branch invocation being replaced instead of
>> a handful. Still a nice speedup, though.
>
> ACK!
>
> Thanks,
> Dscho
Thanks, both. This indeed is a good update.
>
>>
>> t/t3427-rebase-subtree.sh | 5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
>> index 39e348de16..bec48e6a1f 100755
>> --- a/t/t3427-rebase-subtree.sh
>> +++ b/t/t3427-rebase-subtree.sh
>> @@ -59,7 +59,10 @@ test_expect_success 'setup' '
>> test_commit files_subtree/master5 &&
>>
>> git checkout -b to-rebase &&
>> -git filter-branch --prune-empty -f --subdirectory-filter files_subtree
>> &&
>> +git fast-export --no-data HEAD -- files_subtree/ |
>> +sed -e "s%\([0-9a-f]\{40\} \)files_subtree/%\1%" |
>> +git fast-import --force --quiet &&
>> +git reset --hard &&
>> git commit -m "Empty commit" --allow-empty
>> '
>>
>> --
>> 2.22.0.19.ga495766805
>>
>>
[BUG] Password cannot contain hash
Hello, if the password contains a hash, then it's impossible to clone a repository, either with https or ssh protocols. I've had to use a `gitg' GUI to clone such a repo. -- Sebastian Gniazdowski News: https://twitter.com/ZdharmaI IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin Blog: http://zdharma.org
Re: [BUG] Password cannot contain hash
Sebastian Gniazdowski writes: > Hello, > if the password contains a hash, then it's impossible to clone a > repository, either with https or ssh protocols. I've had to use a > `gitg' GUI to clone such a repo. Hmph, do you mean that git clone https://user:passw...@hosting.site/repository/ git clone ssh://user:passw...@hosting.site/repository/ if the "password" has '#' in it? Do passwords with colon, slash or at-sign have the same issue? Would it help to URI escape the problematic characters? It is a separate issue to use passwords embedded in the URLs, but still, escaping ought to work.
Re: [BUG] Password cannot contain hash
On Mon, Sep 09, 2019 at 03:01:32PM -0700, Junio C Hamano wrote:
> Sebastian Gniazdowski writes:
>
> > Hello,
> > if the password contains a hash, then it's impossible to clone a
> > repository, either with https or ssh protocols. I've had to use a
> > `gitg' GUI to clone such a repo.
>
> Hmph, do you mean that
>
> git clone https://user:passw...@hosting.site/repository/
> git clone ssh://user:passw...@hosting.site/repository/
>
> if the "password" has '#' in it? Do passwords with colon, slash or
> at-sign have the same issue? Would it help to URI escape the
> problematic characters?
>
> It is a separate issue to use passwords embedded in the URLs, but
> still, escaping ought to work.
It does need escaped in a URL, or we complain. I wondered if we might be
screwing it up elsewhere in the code, too, but t5550 seems to pass with
the modification below.
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 0d985758c6..a1bc0019c9 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -154,7 +154,7 @@ prepare_httpd() {
HTTPD_DEST=127.0.0.1:$LIB_HTTPD_PORT
HTTPD_URL=$HTTPD_PROTO://$HTTPD_DEST
HTTPD_URL_USER=$HTTPD_PROTO://user%40host@$HTTPD_DEST
- HTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:pass%40host@$HTTPD_DEST
+ HTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:a%23hash@$HTTPD_DEST
if test -n "$LIB_HTTPD_DAV" || test -n "$LIB_HTTPD_SVN"
then
diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
index 99a34d6487..636ba5b9eb 100644
--- a/t/lib-httpd/passwd
+++ b/t/lib-httpd/passwd
@@ -1 +1 @@
-user@host:xb4E8pqD81KQs
+user@host:xgK/KkMxjf9xU
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index b811d89cfd..d1b64068e6 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -65,13 +65,13 @@ test_expect_success 'http auth can use user/pass in URL' '
'
test_expect_success 'http auth can use just user in URL' '
- set_askpass wrong pass@host &&
+ set_askpass wrong "a#hash" &&
git clone "$HTTPD_URL_USER/auth/dumb/repo.git" clone-auth-pass &&
expect_askpass pass user@host
'
test_expect_success 'http auth can request both user and pass' '
- set_askpass user@host pass@host &&
+ set_askpass user@host "a#hash" &&
git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-both &&
expect_askpass both user@host
'
@@ -80,7 +80,7 @@ test_expect_success 'http auth respects credential helper
config' '
test_config_global credential.helper "!f() {
cat >/dev/null
echo username=user@host
- echo password=pass@host
+ echo password="a#hash"
}; f" &&
set_askpass wrong &&
git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-helper &&
@@ -89,21 +89,21 @@ test_expect_success 'http auth respects credential helper
config' '
test_expect_success 'http auth can get username from config' '
test_config_global "credential.$HTTPD_URL.username" user@host &&
- set_askpass wrong pass@host &&
+ set_askpass wrong "a#hash" &&
git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-user &&
expect_askpass pass user@host
'
test_expect_success 'configured username does not override URL' '
test_config_global "credential.$HTTPD_URL.username" wrong &&
- set_askpass wrong pass@host &&
+ set_askpass wrong "a#hash" &&
git clone "$HTTPD_URL_USER/auth/dumb/repo.git" clone-auth-user2 &&
expect_askpass pass user@host
'
test_expect_success 'set up repo with http submodules' '
git init super &&
- set_askpass user@host pass@host &&
+ set_askpass user@host "a#hash" &&
(
cd super &&
git submodule add "$HTTPD_URL/auth/dumb/repo.git" sub &&
@@ -112,21 +112,21 @@ test_expect_success 'set up repo with http submodules' '
'
test_expect_success 'cmdline credential config passes to submodule via clone' '
- set_askpass wrong pass@host &&
+ set_askpass wrong "a#hash" &&
test_must_fail git clone --recursive super super-clone &&
rm -rf super-clone &&
- set_askpass wrong pass@host &&
+ set_askpass wrong "a#hash" &&
git -c "credential.$HTTPD_URL.username=user@host" \
clone --recursive super super-clone &&
expect_askpass pass user@host
'
test_expect_success 'cmdline credential config passes submodule via fetch' '
- set_askpass wrong pass@host &&
+ set_askpass wrong "a#hash" &&
test_must_fail git -C super-clone fetch --recurse-submodules &&
- set_askpass wrong pass@host &&
+ set_askpass wrong "a#hash" &&
git -C super-clone \
-c "credential.$HTTPD_URL.username=user@host" \
fetch --recurse-submodules &&
@@ -140,10 +140,10 @@ test_expect_success 'cmdline credential config passes
submodule update' '
sha1=$(git rev-parse HEAD) &&
git -C
Re: [PATCH v2] cache-tree: do not lazy-fetch merge tree
On Mon, Sep 09, 2019 at 02:05:53PM -0700, Junio C Hamano wrote: > Junio C Hamano writes: > > > Isn't that what is going on? I thought I dug up the original that > > introduced the has_object_file() call to this codepath to make sure > > we understand why we make the check (and I expected the person who > > is proposing this change to do the same and record the finding in > > the proposed log message). > > > > I am running out of time today, and will revisit later this week > > (I'll be down for at least two days starting tomorrow, by the way). > > Here is what I came up with. > > The cache-tree datastructure is used to speed up the comparison > between the HEAD and the index, and when the index is updated by > a cherry-pick (for example), a tree object that would represent > the paths in the index in a directory is constructed in-core, to > see if such a tree object exists already in the object store. > > When the lazy-fetch mechanism was introduced, we converted this > "does the tree exist?" check into an "if it does not, and if we > lazily cloned, see if the remote has it" call by mistake. Since > the whole point of this check is to repair the cache-tree by > recording an already existing tree object opportunistically, we > shouldn't even try to fetch one from the remote. > > Pass the OBJECT_INFO_SKIP_FETCH_OBJECT flag to make sure we only > check for existence in the local object store without triggering the > lazy fetch mechanism. As a third-party observer, that explanation makes sense to me. I wondered also if this means we should be using OBJECT_INFO_QUICK. I.e., do we expect to see a "miss" here often, forcing us to re-scan the packed directory? Reading dd0c34c46b (cache-tree: protect against "git prune"., 2006-04-24), I think the answer is "no". -Peff
Re: [PATCH] ci: install P4 from package to fix build error
Hi, On Fri, 6 Sep 2019, SZEDER Gábor wrote: > On Fri, Sep 06, 2019 at 12:27:11PM +0200, SZEDER Gábor wrote: > > To test 'git-p4' in the Linux Clang and GCC build jobs we used to > > install the 'p4' and 'p4d' binaries by directly downloading those > > binaries from a Perforce filehost. This has worked just fine ever > > since we started using Travis CI [1], but during the last day or so > > that filehost appeared to be gone: while its hostname still resolves, > > the host doesn't seem to reply to any download request, it doesn't > > even refuse the connection, and eventually our build jobs time out > > [2]. > > > > Now, this might be just a temporary glitch, but I'm afraid that it > > isn't. > > Well, now would you believe it, while I was testing this patch (I even > made a gitgitgadget PR to run it on Azure Pipelines! :) and touching > up its log message the good old Perforce filehost sprang back to life, > and the CI build jobs now succeed again even without this patch. Sorry for being so slow with granting you access to GitGitGadget. FWIW _anybody_ who already was granted access can issue `/allow` commands, it is not just me. > > Let's install P4 from the package repository, because this approach > > seems to be simpler and more future proof. > > > > Note that we used to install an old P4 version (2016.2) in the Linux > > build jobs, but with this change we'll install the most recent version > > available in the Perforce package repository (currently 2019.1). > > So I'm not quite sure whether we really want this patch. It depends > on how important it is to test 'git-p4' with an old P4 version, but I > don't really have an opinion on that. I'd rather have that patch. It seems to be a much better idea to use the package management system than to rely on one host, especially when said host already displayed hiccups. Ciao, Dscho
[PATCH 1/1] doc: small formatting fix
From: Cameron Steffen
move an incorrectly placed backtick
Signed-off-by: Cameron Steffen
---
Documentation/pretty-formats.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 079598307a..b87e2e83e6 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -208,7 +208,7 @@ endif::git-rev-list[]
'%GP':: show the fingerprint of the primary key whose subkey was used
to sign a signed commit
'%gD':: reflog selector, e.g., `refs/stash@{1}` or `refs/stash@{2
- minutes ago`}; the format follows the rules described for the
+ minutes ago}`; the format follows the rules described for the
`-g` option. The portion before the `@` is the refname as
given on the command line (so `git log -g refs/heads/master`
would yield `refs/heads/master@{0}`).
--
gitgitgadget
[PATCH 0/1] doc: small formatting fix
Edit: I need permission to submit please Cameron Steffen (1): doc: small formatting fix Documentation/pretty-formats.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 75b2f01a0f642b39b0f29b6218515df9b5eb798e Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-330%2Fcamsteffen%2Fpatch-1-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-330/camsteffen/patch-1-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/330 -- gitgitgadget
Re: What's cooking in git.git (Sep 2019, #01; Sat, 7)
On 2019-09-07 at 17:26:53, Junio C Hamano wrote: > * bc/object-id-part17 (2019-08-19) 26 commits > - midx: switch to using the_hash_algo > - builtin/show-index: replace sha1_to_hex > - rerere: replace sha1_to_hex > - builtin/receive-pack: replace sha1_to_hex > - builtin/index-pack: replace sha1_to_hex > - packfile: replace sha1_to_hex > - wt-status: convert struct wt_status to object_id > - cache: remove null_sha1 > - builtin/worktree: switch null_sha1 to null_oid > - builtin/repack: write object IDs of the proper length > - pack-write: use hash_to_hex when writing checksums > - sequencer: convert to use the_hash_algo > - bisect: switch to using the_hash_algo > - sha1-lookup: switch hard-coded constants to the_hash_algo > - config: use the_hash_algo in abbrev comparison > - combine-diff: replace GIT_SHA1_HEXSZ with the_hash_algo > - bundle: switch to use the_hash_algo > - connected: switch GIT_SHA1_HEXSZ to the_hash_algo > - show-index: switch hard-coded constants to the_hash_algo > - blame: remove needless comparison with GIT_SHA1_HEXSZ > - builtin/rev-parse: switch to use the_hash_algo > - builtin/blame: switch uses of GIT_SHA1_HEXSZ to the_hash_algo > - builtin/receive-pack: switch to use the_hash_algo > - fetch-pack: use parse_oid_hex > - patch-id: convert to use the_hash_algo > - builtin/replace: make hash size independent > > Preparation for SHA-256 upgrade continues. > > Looked mostly OK, with a possible update. > cf. <20190820223606.gj365...@genre.crustytoothpaste.net> Just to update on the status of this, I wasn't planning on a reroll, although I'm happy to do so if folks have feedback. Opinions for or against the current state are welcome. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: What's cooking in git.git (Sep 2019, #01; Sat, 7)
On Mon, Sep 09, 2019 at 12:57:17PM -0700, Junio C Hamano wrote: > Denton Liu writes: > > > Hi Junio, > > > > On Sat, Sep 07, 2019 at 10:26:53AM -0700, Junio C Hamano wrote: > >> * dl/complete-cherry-pick-revert-skip (2019-08-27) 3 commits > >> - status: mention --skip for revert and cherry-pick > >> - completion: add --skip for cherry-pick and revert > >> - completion: merge options for cherry-pick and revert > >> > >> The command line completion support (in contrib/) learned about the > >> "--skip" option of "git revert" and "git cherry-pick". > >> > >> Will merge to 'next'. > > > > Did we end up deciding whether or not we were going to drop "status: > > mention --skip for revert and cherry-pick"? > > If you are not convinced it is a good idea, we can easily drop it > (and I do not mind dropping it---I am not convinced it is a good > idea myself). In that case, let's not drop it. The original impetus for this idea came from cherry-picking a range of commits where one of the commits in the range patched a file that didn't exist on the target branch, so in this case skipping was the right course of action. In any case, I believe that giving the user more information in this case is, at worst, neutral.
Re: What's cooking in git.git (Sep 2019, #01; Sat, 7)
"brian m. carlson" writes: > On 2019-09-07 at 17:26:53, Junio C Hamano wrote: >> * bc/object-id-part17 (2019-08-19) 26 commits >> - midx: switch to using the_hash_algo >> ... >> - builtin/replace: make hash size independent >> >> Preparation for SHA-256 upgrade continues. >> >> Looked mostly OK, with a possible update. >> cf. <20190820223606.gj365...@genre.crustytoothpaste.net> > > Just to update on the status of this, I wasn't planning on a reroll, > although I'm happy to do so if folks have feedback. Opinions for or > against the current state are welcome. FWIW, I am quite pleased with these and deeply appreciate the work you've been doing on this topic. Thanks.
Re: [PATCH v2] cache-tree: do not lazy-fetch merge tree
Jeff King writes: > I wondered also if this means we should be using OBJECT_INFO_QUICK. > I.e., do we expect to see a "miss" here often, forcing us to re-scan the > packed directory? As a performance optimization hack, it is OK if we did not notice that the tree object, which corresponds to what is currently prepared for a directory in the index, does exist in the object store. It is not worth rescanning the packs to "protect" against races, I think, in the "repair" codepath. When the user actually wants to write the index out as a tree, we would write it out as a loose object (or omit doing so if we know there are already copies), but because it is not a crime to create a duplicate loose object when we already have a packed copy, I do not think we need to rescan in that context, either. But I do not think the codepath Jonathan's patch touches is about that operation.
Re: What's cooking in git.git (Sep 2019, #01; Sat, 7)
Hi Junio, On Sun, 8 Sep 2019 at 14:51, Junio C Hamano wrote: > > * ma/asciidoctor-refmiscinfo (2019-09-03) 2 commits > - doc-diff: replace --cut-header-footer with --cut-footer > - asciidoctor-extensions: provide `` > > Update support for Asciidoctor documentation toolchain. > > Will merge to 'next'. Please hold off on this. Asciidoctor changed the way they handle these attributes in version 1.5.7 and my patches are incomplete at best. I developed these with 1.5.5 and now I am also able to test with 1.5.7. I should be able to come up with a series that works with both of those versions. Martin
[RFC PATCH 0/1] for-each-ref: do not output empty lines
Hi, This is a follow-up patch based on conversation from the thread: "[RFC PATCH 1/1] for-each-ref: add '--no-symbolic' option" I had proposed a `--no-symbolic` option to git-for-each-ref that would filter out symbolic refs from the output. It was discovered that the behavior was already possible using an `%(if)` atom, but with the small caveat that empty lines would be output for each non-symbolic ref. This patch changes the output behavior of git-for-each-ref such that it only prints lines for refs for which the format string expands to a non-empty string. If this is deemed to be too disruptive of a change, a new command option could be added to opt in to the new behavior. Cheers Eric Freese (1): for-each-ref: do not output empty lines ref-filter.c| 3 ++- t/t6300-for-each-ref.sh | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) -- 2.23.0
[RFC PATCH 1/1] for-each-ref: do not output empty lines
If the format string expands to an empty string for a given ref, do not
print the empty line.
This is helpful when wanting to print only certain kinds of refs that
you can't already filter for.
For example, to exclude symbolic refs, use format string:
"%(if)%(symref)%(then)%(else)%(refname)%(end)"
Or to include only symbolic refs, use:
"%(if)%(symref)%(then)%(refname)%(end)"
Signed-off-by: Eric Freese
---
ref-filter.c| 3 ++-
t/t6300-for-each-ref.sh | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 7338cfc671..b5b3c4ddf6 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2395,9 +2395,10 @@ void show_ref_array_item(struct ref_array_item *info,
if (format_ref_array_item(info, format, &final_buf, &error_buf))
die("%s", error_buf.buf);
fwrite(final_buf.buf, 1, final_buf.len, stdout);
+ if (final_buf.len)
+ putchar('\n');
strbuf_release(&error_buf);
strbuf_release(&final_buf);
- putchar('\n');
}
void pretty_print_ref(const char *name, const struct object_id *oid,
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 9c910ce746..1f070e63e2 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -42,7 +42,7 @@ test_atom() {
sym) ref=refs/heads/sym ;;
*) ref=$1 ;;
esac
- printf '%s\n' "$3" >expected
+ { test -n "$3" && printf '%s\n' "$3"; } >expected
test_expect_${4:-success} $PREREQ "basic atom: $1 $2" "
git for-each-ref --format='%($2)' $ref >actual &&
sanitize_pgp actual.clean &&
@@ -313,7 +313,7 @@ test_expect_success 'Check format of strftime-local date
fields' '
'
test_expect_success 'exercise strftime with odd fields' '
- echo >expected &&
+ >expected &&
git for-each-ref --format="%(authordate:format:)" refs/heads >actual &&
test_cmp expected actual &&
long="long format --
$ZERO_OID$ZERO_OID$ZERO_OID$ZERO_OID$ZERO_OID$ZERO_OID$ZERO_OID" &&
--
2.23.0
Re: [RFC PATCH 1/1] for-each-ref: do not output empty lines
Eric Freese writes:
> If the format string expands to an empty string for a given ref, do not
> print the empty line.
>
> This is helpful when wanting to print only certain kinds of refs that
> you can't already filter for.
We tend to prefer stating the reason why we want to do so first and
then give a command to the codebase to "become like so". Here is to
illustrate how you would do it:
The custom format specifier "--format=" can be used to
tell the for-each-ref command to say nothing for certain kind of
refs, e.g.
--format="%(if)%(symref)%(then)%(else)%(refname)%(end)"
may be used to show the refname only for refs that are not
symbolic refs. Except that the command still would show one
blank line per each symbolic ref, which is fairly useless.
Introduce the `--omit-empty-lines` option to squelch these
useless lines from the output.
> @@ -2395,9 +2395,10 @@ void show_ref_array_item(struct ref_array_item *info,
> if (format_ref_array_item(info, format, &final_buf, &error_buf))
> die("%s", error_buf.buf);
> fwrite(final_buf.buf, 1, final_buf.len, stdout);
> + if (final_buf.len)
> + putchar('\n');
While we are introducing a conditional, let's drop the useless
fwrite of 0-byte while we are at it [*1*], i.e.
if (final_buf.len && !omit_empty_lines) {
fwrite(final_buf.buf, 1, final_buf.len, stdout);
putchar('\n');
}
Thanks.
[Footnote]
*1* "While we are at it", the existing code tempts me to drop fwrite
and replace it with something along the lines of...
printf("%*s\n", count, buf)
but I refrained from doing so. An enhancement patch like this
is not a place to "improve" existing code (which should be done
as a separate patch).
Re: [RFC PATCH 1/1] for-each-ref: do not output empty lines
Junio C Hamano writes:
>> fwrite(final_buf.buf, 1, final_buf.len, stdout);
>> +if (final_buf.len)
>> +putchar('\n');
>
> While we are introducing a conditional, let's drop the useless
> fwrite of 0-byte while we are at it [*1*], i.e.
>
> if (final_buf.len && !omit_empty_lines) {
Of course, that's a typo for "||"; if it is not empty, we'd emit no
matter what, and if omit_empty is not given, we'd emit whether it is
empty or not.
> fwrite(final_buf.buf, 1, final_buf.len, stdout);
> putchar('\n');
> }
>
> Thanks.
>
>
> [Footnote]
>
> *1* "While we are at it", the existing code tempts me to drop fwrite
> and replace it with something along the lines of...
>
> printf("%*s\n", count, buf)
>
> but I refrained from doing so. An enhancement patch like this
> is not a place to "improve" existing code (which should be done
> as a separate patch).
[PATCH 1/1] Fix perl error "unescaped left brace in regex" for paranoid update hook
From: Dominic Winkler
A literal "{" should now be escaped in a pattern starting from perl
versions >= v5.26. In perl v5.22, using a literal { in a regular
expression was deprecated, and will emit a warning if it isn't escaped: \{.
In v5.26, this won't just warn, it'll cause a syntax error.
(see https://metacpan.org/pod/release/RJBS/perl-5.22.0/pod/perldelta.pod)
Signed-off-by: Dominic Winkler
---
contrib/hooks/update-paranoid | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/hooks/update-paranoid b/contrib/hooks/update-paranoid
index d18b317b2f..fc0a242a4e 100755
--- a/contrib/hooks/update-paranoid
+++ b/contrib/hooks/update-paranoid
@@ -302,13 +302,13 @@ $op = 'U' if ($op eq 'R'
RULE:
foreach (@$rules) {
- while (/\${user\.([a-z][a-zA-Z0-9]+)}/) {
+ while (/\$\{user\.([a-z][a-zA-Z0-9]+)}/) {
my $k = lc $1;
my $v = $data{"user.$k"};
next RULE unless defined $v;
next RULE if @$v != 1;
next RULE unless defined $v->[0];
- s/\${user\.$k}/$v->[0]/g;
+ s/\$\{user\.$k}/$v->[0]/g;
}
if (/^([AMD
]+)\s+of\s+([^\s]+)\s+for\s+([^\s]+)\s+diff\s+([^\s]+)$/) {
--
gitgitgadget
[PATCH 0/1] Fix perl error "unescaped left brace in regex" for paranoid update hook
A literal "{" should now be escaped in a pattern starting from perl versions
>= v5.26. In perl v5.22, using a literal { in a regular expression was
deprecated, and will emit a warning if it isn't escaped: {. In v5.26, this
won't just warn, it'll cause a syntax error.
(see https://metacpan.org/pod/release/RJBS/perl-5.22.0/pod/perldelta.pod)
Signed-off-by: Dominic Winkler d.wink...@flexarts.at [d.wink...@flexarts.at]
Dominic Winkler (1):
Fix perl error "unescaped left brace in regex" for paranoid update
hook
contrib/hooks/update-paranoid | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As:
https://github.com/gitgitgadget/git/releases/tag/pr-335%2Fflexarts%2Fmaint-update-paranoid-perlv5.26-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-335/flexarts/maint-update-paranoid-perlv5.26-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/335
--
gitgitgadget
