Re: [PATCH] rebase -i: introduce the 'test' command
Hi Peff, On Mon, 3 Dec 2018, Jeff King wrote: > On Mon, Dec 03, 2018 at 08:01:44PM +0100, Johannes Schindelin wrote: > > > > In this sort of situation, I often whish to be able to do nested rebases. > > > Even more because it happen relatively often that I forget that I'm > > > working in a rebase and not on the head, and then it's quite natural > > > to me to type things like 'git rebase -i @^^^' while already rebasing. > > > But I suppose this has already been discussed. > > > > Varieties of this have been discussed, but no, not nested rebases. > > > > The closest we thought about was re-scheduling the latest commits, > > which is now harder because of the `--rebase-merges` mode. > > > > But I think it would be doable. Your idea of a "nested" rebase actually > > opens that door quite nicely. It would not *really* be a nested rebase, > > and it would still only be possible in interactive mode, but I could > > totally see > > > > git rebase --nested -i HEAD~3 > > > > to generate and prepend the following lines to the `git-rebase-todo` file: > > > > reset abcdef01 # This is HEAD~3 > > pick abcdef02 # This is HEAD~2 > > pick abcdef03 # This is HEAD~ > > pick abcdef04 # This is HEAD > > > > (assuming that the latest 3 commits were non-merge commits; It would look > > quite a bit more complicated in other situations.) > > Yeah, I would probably use that if it existed. I kind of use it, even if it does not exist ;-) > It would be nicer to have real nested sequencer operations, I think, for > other situations. I agree. But for the moment, our data format is too married to the exact layout of .git/, thanks to `git rebase`'s evolution from a Unix shell script. Alban has this really great patch series to work on the todo list in-memory, and that paves the way to decouple the entire sequencer thing from the file system. The most notably thing that still would need to be encapsulated would be the options: currently, there is a plethora of inconsistent options files being saved into the state directory (for some, the mere presence indicates `true`, some contain `true` or `false`, others contain text, etc). > E.g., cherry-picking a sequence of commits while you're in the middle of > a rebase. You will be delighted to learn that you can cherry-pick a sequence of commits in the middle of a rebase already. I do `exec git cherry-pick ` *all* the time. > But I suspect getting that right would be _loads_ more work, and > probably would involve some funky UI corner cases to handle the stack of > operations (so truly aborting a rebase may mean an arbitrary number of > "rebase --abort" calls to pop the stack). Your suggestion is probably a > reasonable trick in the meantime. You know what is an even more reasonable trick? Worktrees. I only thought about that this morning, but I should have mentioned it right away, as I use it quite frequently. When I have tricky nested rebases to perform, I do use throw-away worktrees where I check out unnamed branches, work on those, and then integrate them back into the "outer rebase" via the `reset` command in the todo list. Ciao, Dscho
Re: [PATCH] t/lib-git-daemon: fix signal checking
On Mon, Nov 26, 2018 at 09:03:37PM +0100, SZEDER Gábor wrote: > Test scripts checking 'git daemon' stop the daemon with a TERM signal, > and the 'stop_git_daemon' helper checks the daemon's exit status to > make sure that it indeed died because of that signal. > > This check is bogus since 03c39b3458 (t/lib-git-daemon: use > test_match_signal, 2016-06-24), for two reasons: > > - Right after killing 'git daemon', 'stop_git_daemon' saves its exit > status in a variable, but since 03c39b3458 the condition checking > the exit status looks at '$?', which at this point is not the exit > status of 'git daemon', but that of the variable assignment, i.e. > it's always 0. > > - The unexpected exit status should abort the whole test script with > 'error', but it doesn't, because 03c39b3458 forgot to negate > 'test_match_signal's exit status in the condition. > > This patch fixes both issues. Oof. Who says two wrongs don't make a right? :) Thanks for catching this, and the patch looks obviously correct. I peeked at the other test_match_signal conversions from that era, and they all look sane. -Peff
Re: [PATCH 1/3] RelNotes 2.20: move some items between sections
On Tue, 4 Dec 2018 at 03:23, Junio C Hamano wrote: > > Martin Ågren writes: > > > Some items that should be in "Performance, Internal Implementation, > > Development Support etc." have ended up in "UI, Workflows & Features" > > and "Fixes since v2.19". Move them, and do s/uses/use/ while at it. > > I agree with the early half of this change; I think it is OK to > consider lack of preparation for Travis transition and lack of > better testing in the maintenance track as bugs, though. Sure. Here's a resend where patch 1/3 has been simplified accordingly. Martin Ågren (3): RelNotes 2.20: move some items between sections RelNotes 2.20: clarify sentence RelNotes 2.20: drop spurious double quote Documentation/RelNotes/2.20.0.txt | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) Range-diff against v1: 1: d69f63b5f6 ! 1: 961bfc2ad6 RelNotes 2.20: move some items between sections @@ -3,8 +3,8 @@ RelNotes 2.20: move some items between sections Some items that should be in "Performance, Internal Implementation, -Development Support etc." have ended up in "UI, Workflows & Features" -and "Fixes since v2.19". Move them, and do s/uses/use/ while at it. +Development Support etc." have ended up in "UI, Workflows & Features". +Move them, and do s/uses/use/ while at it. Signed-off-by: Martin Ågren @@ -35,33 +35,3 @@ * When there are too many packfiles in a repository (which is not recommended), looking up an object in these would require consulting many pack .idx files; a new mechanism to have a single -@@ -two classes to ease code migration process has been proposed and -its support has been added to the Makefile. - -+ * The "container" mode of TravisCI is going away. Our .travis.yml -+ file is getting prepared for the transition. -+ (merge 32ee384be8 ss/travis-ci-force-vm-mode later to maint). -+ -+ * Our test scripts can now take the '-V' option as a synonym for the -+ '--verbose-log' option. -+ (merge a5f52c6dab sg/test-verbose-log later to maint). -+ - - Fixes since v2.19 - - -@@ -didn't make much sense. This has been corrected. -(merge 669b1d2aae md/exclude-promisor-objects-fix later to maint). - -- * The "container" mode of TravisCI is going away. Our .travis.yml -- file is getting prepared for the transition. -- (merge 32ee384be8 ss/travis-ci-force-vm-mode later to maint). -- -- * Our test scripts can now take the '-V' option as a synonym for the -- '--verbose-log' option. -- (merge a5f52c6dab sg/test-verbose-log later to maint). -- - * A regression in Git 2.12 era made "git fsck" fall into an infinite -loop while processing truncated loose objects. -(merge 18ad13e5b2 jk/detect-truncated-zlib-input later to maint). 2: eccb7edd08 = 2: 3027a34938 RelNotes 2.20: clarify sentence 3: 78f3043b65 = 3: a5e2df91b4 RelNotes 2.20: drop spurious double quote -- 2.20.0.rc2.1.gc81af441bb
Re: [PATCH v3] range-diff: always pass at least minimal diff options
On Mon, 3 Dec 2018 at 22:21, Eric Sunshine wrote:
> [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).
Thank you so much for this.
> if (rev->rdiff1) {
> + /*
> +* Pass minimum required diff-options to range-diff; others
> +* can be added later if deemed desirable.
> +*/
Agreed.
> + struct diff_options opts;
> + diff_setup();
> + opts.file = rev->diffopt.file;
> + opts.use_color = rev->diffopt.use_color;
Ah, s/0/rev->diffopt.use_color/, well that's obvious.
Thanks!
Martin
Re: sharedrepository=group not working
On Dec 3, 2018, at 8:50 PM, Jeff King wrote: > > I don't suppose this is leaving those incoming-* directories sitting > around so we can inspect their permissions (it's suppose to clean them > up, so I doubt it). If you're up for it, it might be interesting to > patch Git to inspect the umask and "ls -l" the objects/ directory at the > problematic moment. The interesting point is when we call into > tmp-objdir.c:setup_tmp_objdir(). The problem was that Apache was setting my umask to 113! With that: + ls -ldF ./objects/incoming-w7agmb/pack objects/incoming-w7agmb ls: cannot access ./objects/incoming-w7agmb/pack: Permission denied drw---S--- 2 apache cvs 4096 Dec 3 21:14 objects/incoming-w7agmb/ error: remote unpack failed: unable to create temporary object directory With 002 it succeeds: + ls -ldF ./objects/incoming-IbGS6h/pack objects/incoming-IbGS6h drwx--S--- 3 apache cvs 4096 Dec 3 21:19 objects/incoming-IbGS6h/ drwxrwsr-x 2 apache cvs 4096 Dec 3 21:19 ./objects/incoming-IbGS6h/pack/ So I fixed my umask and got it working, but maybe a test for "your umask is dumb" is worthwhile. Thanks for your help! -- Jamie Zawinski https://www.jwz.org/ https://www.dnalounge.com/
Re: sharedrepository=group not working
On Mon, Dec 03, 2018 at 08:19:12PM -0800, Jamie Zawinski wrote: > On Dec 3, 2018, at 8:09 PM, Jeff King wrote: > > > > but it works fine. Might there be some effective-uid trickiness with the > > way the server side of git is invoked? Or is this a network mount where > > the filesystem uid might not match the process uid? > > Huh. They're on the same ext4 fs (it's an AWS EBS sc1 volume, but I > think that still counts as "not a network mount" as far as Linux is > concerned.) Yeah, I think we can discount any oddness there. > The way I was seeing this fail was a CGI invoking "git push", as user > "httpd" (and I verified that when the cgi was invoked, "groups" > reported that "httpd" was a member of group "cvs") but when I tried to > reproduce the error with "sudo -u apache git push" it didn't fail. So > possibly something hinky is going on with group permissions when httpd > invokes git, but I did verify that whoami, groups and pwd were as > expected, so I couldn't tell what that might be... (Oh, I didn't check > what umask was, but it should have been 022...) Hrm. I don't think group permissions would even matter. We asked to mkdir() with 0700 anyway, so we know they'd be zero. But a funny umask does seem like a likely candidate for causing the problem. We asked for 0700, but if there were bits set in umask (say, 0200 or something), that would restrict that further. And it would explain what you're seeing (inability to write into a directory we just created), and it might have worked with previous versions (which was less strict on the group permissions). I don't suppose this is leaving those incoming-* directories sitting around so we can inspect their permissions (it's suppose to clean them up, so I doubt it). If you're up for it, it might be interesting to patch Git to inspect the umask and "ls -l" the objects/ directory at the problematic moment. The interesting point is when we call into tmp-objdir.c:setup_tmp_objdir(). -Peff
Re: sharedrepository=group not working
On Dec 3, 2018, at 8:19 PM, Jamie Zawinski wrote: > > (Oh, I didn't check what umask was, but it should have been 022...) Typo, I mean to say 002. -- Jamie Zawinski https://www.jwz.org/ https://www.dnalounge.com/
Re: sharedrepository=group not working
On Dec 3, 2018, at 8:09 PM, Jeff King wrote: > > but it works fine. Might there be some effective-uid trickiness with the > way the server side of git is invoked? Or is this a network mount where > the filesystem uid might not match the process uid? Huh. They're on the same ext4 fs (it's an AWS EBS sc1 volume, but I think that still counts as "not a network mount" as far as Linux is concerned.) The way I was seeing this fail was a CGI invoking "git push", as user "httpd" (and I verified that when the cgi was invoked, "groups" reported that "httpd" was a member of group "cvs") but when I tried to reproduce the error with "sudo -u apache git push" it didn't fail. So possibly something hinky is going on with group permissions when httpd invokes git, but I did verify that whoami, groups and pwd were as expected, so I couldn't tell what that might be... (Oh, I didn't check what umask was, but it should have been 022...) -- Jamie Zawinski https://www.jwz.org/ https://www.dnalounge.com/
Re: sharedrepository=group not working
On Mon, Dec 03, 2018 at 07:27:13PM -0800, Jamie Zawinski wrote:
> I think sharedrepository=group stopped working some time between
> 2.10.5 (works) and 2.12.4 (does not). 2.19.2 also does not.
Hmm. Given the time-frame and the fact that your strace shows problems
writing into the objects/incoming-* directory, it's likely caused by
722ff7f876 (receive-pack: quarantine objects until pre-receive accepts,
2016-10-03).
The big change there is that instead of writing directly into objects/,
we create a temporary objects/incoming-* directory, write there, and
then migrate the objects over after we determine they're sane.
So in your strace we see the temp directory get created:
> mkdir("./objects/incoming-U5EN8D", 0700
> <... mkdir resumed> ) = 0
The permissions are tighter than we ultimately want, but that's OK.
This tempdir is just for this process (and its children) to look at, and
then we'd eventually migrate the files out.
I could definitely imagine there being a bug in which we don't then
properly loosen permissions when we move things out of the tempdir, but
we don't even get that far. We fail immediately:
> mkdir("./objects/incoming-U5EN8D/pack", 0777) = -1 EACCES (Permission denied)
That seems strange. The outer directory is only 0700, but the user
permissions should be sufficient. Even with the g+s bit set, it should
still be owned by the same user, shouldn't it?
I tried reproducing your state like this:
git init --bare dst.git
git -C dst.git config core.sharedrepository group
chgrp -R somegroup dst.git
find dst.git -type f | xargs chmod g+rw
find dst.git -type d | xargs chmod g+srw
# push works from original user
git clone dst.git client
(
cd client &&
git commit --allow-empty -m foo
git push
)
# push works from alternate user
sudo su anotheruser sh -c '
git clone dst.git /tmp/other &&
cd /tmp/other &&
git commit --allow-empty -m foo &&
git push --receive-pack="strace -e mkdir git-receive-pack"
'
but it works fine. Might there be some effective-uid trickiness with the
way the server side of git is invoked? Or is this a network mount where
the filesystem uid might not match the process uid?
-Peff
Re: [PATCH v3 07/14] checkout: split into switch-branch and restore-files
Elijah Newren writes: >> +Updates files in the working tree to match the version in the index >> +or the specified tree. >> + >> +'git restore-files' [--from=] ...:: > > and ? I understand and , > or but have no clue why it'd be okay to specify > and together. What does that even mean? I have this tree object v2.6.11-tree that is not enclosed in a commit object. I want to take the top-level Makefile out of that tree, stuff it in the index and overwrite the working tree file. $ git checkout v2.6.11-tree Makefile $ git restore-files --from=v2.6.11-tree Makefile >> + Overwrite paths in the working tree by replacing with the >> + contents in the index or in the (most often a >> + commit). When a is given, the paths that >> + match the are updated both in the index and in >> + the working tree. > > Is that the default we really want for this command? Why do we > automatically assume these files are ready for commit? I understand > that it's what checkout did, but I'd find it more natural to only > affect the working tree by default. We can give it an option for > affecting the index instead (or perhaps in addition). Oooah. Now this is getting juicy. I do think supporting "--index" (which would make it more in line with what Duy wrote), with optionally "--cached" as well, and making the "working tree only" mode as default may not be a bad idea. I am offhand not sure how the "working tree only" mode (similar to the default mode of "git apply" that mimics the way "patch -p1" works) should interact with the non-overlay mode of the command, but other than that, I tend to agree with the idea that restore-files is only a part of making the contents into committable shape, not exactly ready for it yet.
Re: [PATCH] sideband: color lines with keyword only
Jonathan Nieder writes:
> Stefan Beller wrote:
>> On Mon, Dec 3, 2018 at 3:23 PM Jonathan Nieder wrote:
>
>>> I was curious about what versions of Gerrit this is designed to
>>> support (or in other words whether it's a bug fix or a feature).
Well, bf1a11f0 ("sideband: highlight keywords in remote sideband
output", 2018-08-07) clearly wanted to allow a keyword followed by
anything !isalnum() to be painted, and we accepted that change
because we thought it was a good idea, so anything that made a
keyword alone not to be painted is a bug, isn't it? Whether output
lines from Gerrit benefits from this fix is a different matter, of
course.
> No worries. Can't hurt for Junio to have a few patches to apply to
> "pu" or "next" to practice using the release candidates. :)
This change falls into "an obvious and small fix to a bug that went
unnoticed and is in an older release (2.19)" category, which is not
eligible for the upcoming release this late in the cycle. I think
enough eyeballs looked at the change already, so let's not waste the
already-spent review braincycle and mark it as "Will merge to 'next'".
Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences
Johannes Sixt writes: > Am 03.12.18 um 21:42 schrieb Martin Ågren: >> On Mon, 3 Dec 2018 at 18:35, Johannes Sixt wrote: >>> I actually did not test the result, because I don't have the >>> infrastructure. >> >> I've tested with asciidoc and Asciidoctor, html and man-page. Looks >> good. > > Thank you so much! > > -- Hannes Thanks, both.
Re: [RFC] git clean --local
Junio C Hamano writes: > If "git clean" takes a pathspec, perhaps you can give a negative > pathspec to exclude whatever you do not want to get cleaned, > something like > > git clean '*.o' ':!precious.o' > > to say "presious.o is ignored (hence normally expendable), but I do > not want to clean it with this invocation of 'git clean'"? Hmph, this leads me to an interesting thought. With today's code, these two commands behave in meaningfully different ways when I mark some paths that match .gitignore patterns with the precious attribute. echo "*.ignored" >>.git/info/exclude echo "precious.* precious" >>.git/info/attributes : >expendable.ignored 2>precious.ignored git clean -n -x git clean -n -x ':(exclude,attr:precious)' I am not suggesting that giving "git clean" a configuration knob that always append pathspec elements, which would allow users to use the mechanism to set the above magic pathspec, would be a good approach. If we were to follow through this line of thought, an obvious thing to do is to always unconditonally append the above magic pathspec internally when running "git clean", which would mean * Existing projects and users' repositories will see no behaviour change, because they are unaware of the "precious" attribute. * People who learn the new feature can start using the "ignored but precious" class, without any need for transition period.
Re: [PATCH 3/3] RelNotes 2.20: drop spurious double quote
Martin Ågren writes: > We have three double-quote characters, which is one too many or too few. > Dropping the last one seems to match the original intention best. Thanks for spotting. The actual original intention was that the user says two things: first saying "add only what does not match '*' out of all branches" and then saying "add all branches, without any exclusion this time". But letting the user first say one thing and then doing another thing without saying it is also fine, which is what your version is. > > Signed-off-by: Martin Ågren > --- > Documentation/RelNotes/2.20.0.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/RelNotes/2.20.0.txt > b/Documentation/RelNotes/2.20.0.txt > index 201135d80c..e71fe3dee1 100644 > --- a/Documentation/RelNotes/2.20.0.txt > +++ b/Documentation/RelNotes/2.20.0.txt > @@ -578,7 +578,7 @@ Fixes since v2.19 > > * "git rev-parse --exclude=* --branches --branches" (i.e. first > saying "add only things that do not match '*' out of all branches" > - and then adding all branches, without any exclusion this time") > + and then adding all branches, without any exclusion this time) > worked as expected, but "--exclude=* --all --all" did not work the > same way, which has been fixed. > (merge 5221048092 ag/rev-parse-all-exclude-fix later to maint).
Re: [PATCH 2/3] RelNotes 2.20: clarify sentence
Martin Ågren writes: > I had to read this sentence a few times to understand it. Let's try to > clarify it. Great. Thanks. > > Signed-off-by: Martin Ågren > --- > Documentation/RelNotes/2.20.0.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/RelNotes/2.20.0.txt > b/Documentation/RelNotes/2.20.0.txt > index e5ab8cc609..201135d80c 100644 > --- a/Documentation/RelNotes/2.20.0.txt > +++ b/Documentation/RelNotes/2.20.0.txt > @@ -305,7 +305,7 @@ Performance, Internal Implementation, Development Support > etc. > > * The overly large Documentation/config.txt file have been split into > million little pieces. This potentially allows each individual piece > - included into the manual page of the command it affects more easily. > + to be included into the manual page of the command it affects more easily. > > * Replace three string-list instances used as look-up tables in "git > fetch" with hashmaps.
Re: [PATCH 1/3] RelNotes 2.20: move some items between sections
Martin Ågren writes: > Some items that should be in "Performance, Internal Implementation, > Development Support etc." have ended up in "UI, Workflows & Features" > and "Fixes since v2.19". Move them, and do s/uses/use/ while at it. > > Signed-off-by: Martin Ågren > --- I agree with the early half of this change; I think it is OK to consider lack of preparation for Travis transition and lack of better testing in the maintenance track as bugs, though. > Documentation/RelNotes/2.20.0.txt | 26 +- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/Documentation/RelNotes/2.20.0.txt > b/Documentation/RelNotes/2.20.0.txt > index b1deaf37da..e5ab8cc609 100644 > --- a/Documentation/RelNotes/2.20.0.txt > +++ b/Documentation/RelNotes/2.20.0.txt > @@ -137,11 +137,6 @@ UI, Workflows & Features > command line, or setting sendemail.suppresscc configuration > variable to "misc-by", can be used to disable this behaviour. > > - * Developer builds now uses -Wunused-function compilation option. > - > - * One of our CI tests to run with "unusual/experimental/random" > - settings now also uses commit-graph and midx. > - > * "git mergetool" learned to take the "--[no-]gui" option, just like > "git difftool" does. > > @@ -185,6 +180,11 @@ UI, Workflows & Features > > Performance, Internal Implementation, Development Support etc. > > + * Developer builds now use -Wunused-function compilation option. > + > + * One of our CI tests to run with "unusual/experimental/random" > + settings now also uses commit-graph and midx. > + > * When there are too many packfiles in a repository (which is not > recommended), looking up an object in these would require > consulting many pack .idx files; a new mechanism to have a single > @@ -387,6 +387,14 @@ Performance, Internal Implementation, Development > Support etc. > two classes to ease code migration process has been proposed and > its support has been added to the Makefile. > > + * The "container" mode of TravisCI is going away. Our .travis.yml > + file is getting prepared for the transition. > + (merge 32ee384be8 ss/travis-ci-force-vm-mode later to maint). > + > + * Our test scripts can now take the '-V' option as a synonym for the > + '--verbose-log' option. > + (merge a5f52c6dab sg/test-verbose-log later to maint). > + > > Fixes since v2.19 > - > @@ -544,14 +552,6 @@ Fixes since v2.19 > didn't make much sense. This has been corrected. > (merge 669b1d2aae md/exclude-promisor-objects-fix later to maint). > > - * The "container" mode of TravisCI is going away. Our .travis.yml > - file is getting prepared for the transition. > - (merge 32ee384be8 ss/travis-ci-force-vm-mode later to maint). > - > - * Our test scripts can now take the '-V' option as a synonym for the > - '--verbose-log' option. > - (merge a5f52c6dab sg/test-verbose-log later to maint). > - > * A regression in Git 2.12 era made "git fsck" fall into an infinite > loop while processing truncated loose objects. > (merge 18ad13e5b2 jk/detect-truncated-zlib-input later to maint).
Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed
Jeff King writes:
> That said, our C99 designated initializer weather-balloons haven't
> gotten any complaints yet. So I think you could actually do:
>
> struct setup_revision_opt s_r_opt = {
> .allow_exclude_promisor_objects = 1,
> };
> ...
> setup_revisions(...);
>
> which is pretty nice.
Yup. The output from
$ git grep -n ' \.[a-z0-9_]* =' -- \*.[ch]
with a bit of "git blame" tells us that cbc0f81d ("strbuf: use
designated initializers in STRBUF_INIT", 2017-07-10) is the balloon
for this exact feature. The same for array was done in 512f41cf
("clean.c: use designated initializer", 2017-07-14)
[I am writing it down so that I do not have to dig for it every time
and instead can ask the list archive]
Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc
On Mon, Dec 03, 2018 at 03:37:35PM -0800, Jonathan Tan wrote: > Signed-off-by: Jonathan Tan > --- > Documentation/technical/packfile-uri.txt | 83 > Documentation/technical/protocol-v2.txt | 6 +- > 2 files changed, 88 insertions(+), 1 deletion(-) > create mode 100644 Documentation/technical/packfile-uri.txt > > diff --git a/Documentation/technical/packfile-uri.txt > b/Documentation/technical/packfile-uri.txt > new file mode 100644 > index 00..6535801486 > --- /dev/null > +++ b/Documentation/technical/packfile-uri.txt > @@ -0,0 +1,83 @@ > +Packfile URIs > += > + > +This feature allows servers to serve part of their packfile response as URIs. > +This allows server designs that improve scalability in bandwidth and CPU > usage > +(for example, by serving some data through a CDN), and (in the future) > provides > +some measure of resumability to clients. > + > +This feature is available only in protocol version 2. > + > +Protocol > + > + > +The server advertises `packfile-uris`. > + > +If the client replies with the following arguments: > + > + * packfile-uris > + * thin-pack > + * ofs-delta > + > +when the server sends the packfile, it MAY send a `packfile-uris` section > +directly before the `packfile` section (right after `wanted-refs` if it is > +sent) containing HTTP(S) URIs. See protocol-v2.txt for the documentation of > +this section. > + > +Clients then should understand that the returned packfile could be > incomplete, > +and that it needs to download all the given URIs before the fetch or clone is > +complete. Each URI should point to a Git packfile (which may be a thin pack > and > +which may contain offset deltas). Some thoughts here: First, I'd like to see a section (and a bit in the implementation) requiring HTTPS if the original protocol is secure (SSH or HTTPS). Allowing the server to downgrade to HTTP, even by accident, would be a security problem. Second, this feature likely should be opt-in for SSH. One issue I've seen repeatedly is that people don't want to use HTTPS to fetch things when they're using SSH for Git. Many people in corporate environments have proxies that break HTTP for non-browser use cases[0], and using SSH is the only way that they can make a functional Git connection. Third, I think the server needs to be required to both support Range headers and never change the content of a URI, so that we can have resumable clone implicit in this design. There are some places in the world where connections are poor and fetching even the initial packfile at once might be a problem. (I've seen such questions on Stack Overflow, for example.) Having said that, I think overall this is a good idea and I'm glad to see a proposal for it. [0] For example, a naughty-word filter may corrupt or block certain byte sequences that occur incidentally in the pack stream. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH v3] range-diff: always pass at least minimal diff options
Eric Sunshine writes: > 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. Yup. Thanks. The only thing that makes me wonder is why any of the existing tests (among which I think I saw range-diff driven from format-patch) did not catch this rather obvious glitch. > 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 && Ah, of course. Then the "actual" file gets the names of the output files; we expect to see 5 of them. But now we have lost all the range-diff tests run under "--stdout", so next time some other change regresses only that codepath, we will not notice (which is fine for now but would want to be addressed before the end of the year around which time we certainly will all forget). Thanks. > - grep "= 1: .* s/5/A" actual && > - grep "= 2: .* s/4/A" actual && > - grep "= 3: .* s/11/B" actual && > - grep "= 4: .* s/12/B" actual > + test_when_finished "rm 000?-*" && > + test_line_count = 5 actual && > + test_i18ngrep "^Range-diff:$" -* && > + grep "= 1: .* s/5/A" -* && > + grep "= 2: .* s/4/A" -* && > + grep "= 3: .* s/11/B" -* && > + grep "= 4: .* s/12/B" -* > ' > done > > test_expect_success 'format-patch --range-diff as commentary' ' > - git format-patch --stdout --range-diff=HEAD~1 HEAD~1 >actual && > - test_i18ngrep "^Range-diff:$" actual > + git format-patch --range-diff=HEAD~1 HEAD~1 >actual && > + test_when_finished "rm 0001-*" && > + test_line_count = 1 actual && > + test_i18ngrep "^Range-diff:$" 0001-* && > + grep "> 1: .* new message" 0001-* > ' > > test_done
Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files
On Thu, Nov 29, 2018 at 2:01 PM Nguyễn Thái Ngọc Duy wrote: > > v3 sees switch-branch go back to switch-branch (in v2 it was > checkout-branch). checkout-files is also renamed restore-files (v1 was > restore-paths). Hopefully we won't see another rename. I started reading through the patches. I also tried to apply them locally, but they had conflicts or missing base file version on both master and next. What version did you base it on? I stopped at 07/14, and dropped my comments all there. I didn't read any further yet, and may wait for your post-2.20 reroll. > I'll try to summarize the differences between the new commands and > 'git checkout' down here, but you're welcome to just head to 07/14 and > read the new man pages. > > 'git switch-branch' > > - does not "do nothing", you have to either switch branch, create a > new branch, or detach. "git switch-branch" with no arguments is > rejected. > > - implicit detaching is rejected. If you need to detach, you need to > give --detach. Or stick to 'git checkout'. > > - -b/-B is renamed to -c/-C with long option names > > - of course does not accept pathspec > > 'git restore-files' > > - takes a ref from --from argument, not as a free ref. As a result, > '--' is no longer needed. All non-option arguments are pathspec > > - pathspec is mandatory, you can't do "git restore-files" without any > pathspec. > > - I just remember -p which is allowed to take no pathspec :( I'll fix > it later. This all looks good. I commented elsewhere but please remember that pathspec implies directories as a possibility and we really need to fix the broken behavior of checkout when given a directory. > - Two more fancy features (the "git checkout --index" being the > default mode and the backup log for accidental overwrites) are of > course still missing. But they are coming. > > I did not go replace "detached HEAD" with "unnamed branch" (or "no > branch") everywhere because I think a unique term is still good to > refer to this concept. Or maybe "no branch" is good enough. I dunno. I personally like "unnamed branch", but "no branch" would still be better than "detached HEAD".
Re: [PATCH v3 07/14] checkout: split into switch-branch and restore-files
On Thu, Nov 29, 2018 at 2:03 PM Nguyễn Thái Ngọc Duy wrote: > > "git checkout" doing too many things is a source of confusion for many > users (and it even bites old timers sometimes). To rememdy that, the > command is now split in two: switch-branch and checkout-files. The "checkout-files" here(will comment more on this below) > good old "git checkout" command is still here and will be until all > (or most of users) are sick of it. > > See the new man pages for the final design of these commands. The > actual implementation though is still pretty much the same as "git > checkout". Following patches will adjust their behavior to match the > man pages. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > .gitignore | 2 + > Documentation/git-checkout.txt | 5 + > Documentation/git-restore-files.txt | 167 > Documentation/git-switch-branch.txt | 289 > Makefile| 2 + > builtin.h | 2 + > builtin/checkout.c | 84 ++-- > command-list.txt| 2 + > git.c | 2 + > 9 files changed, 543 insertions(+), 12 deletions(-) > create mode 100644 Documentation/git-restore-files.txt > create mode 100644 Documentation/git-switch-branch.txt > > diff --git a/.gitignore b/.gitignore > index 0d77ea5894..c63dcb1427 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -143,6 +143,7 @@ > /git-request-pull > /git-rerere > /git-reset > +/git-restore-files ...and "restore-files" here. Should be consistent with whatever name you pick. > /git-rev-list > /git-rev-parse > /git-revert > @@ -167,6 +168,7 @@ > /git-submodule > /git-submodule--helper > /git-svn > +/git-switch-branch > /git-symbolic-ref > /git-tag > /git-unpack-file > diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt > index 25887a6087..25ec7f508f 100644 > --- a/Documentation/git-checkout.txt > +++ b/Documentation/git-checkout.txt > @@ -406,6 +406,11 @@ $ edit frotz > $ git add frotz > > > +SEE ALSO > + > +linkgit:git-switch-branch[1] > +linkgit:git-restore-files[1] > + > GIT > --- > Part of the linkgit:git[1] suite > diff --git a/Documentation/git-restore-files.txt > b/Documentation/git-restore-files.txt > new file mode 100644 > index 00..03c1250ad0 > --- /dev/null > +++ b/Documentation/git-restore-files.txt > @@ -0,0 +1,167 @@ > +git-restore-files(1) > + > + > +NAME > + > +git-restore-files - Restore working tree files > + > +SYNOPSIS > + > +[verse] > +'git restore-files'
Re: [WIP RFC 3/5] upload-pack: refactor reading of pack-objects out
On Mon, Dec 3, 2018 at 3:37 PM Jonathan Tan wrote: > > Subsequent patches will change how the output of pack-objects is > processed, so extract that processing into its own function. > > Currently, at most 1 character can be buffered (in the "buffered" local > variable). One of those patches will require a larger buffer, so replace > that "buffered" local variable with a buffer array. This buffering sounds oddly similar to the pkt reader which can buffer at most one pkt, the difference being that we'd buffer bytes instead of pkts.
Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc
Thanks for bringing this design to the list!
> diff --git a/Documentation/technical/protocol-v2.txt
> b/Documentation/technical/protocol-v2.txt
> index 345c00e08c..2cb1c41742 100644
> --- a/Documentation/technical/protocol-v2.txt
> +++ b/Documentation/technical/protocol-v2.txt
> @@ -313,7 +313,8 @@ header. Most sections are sent only when the packfile is
> sent.
>
> output = acknowledgements flush-pkt |
> [acknowledgments delim-pkt] [shallow-info delim-pkt]
> -[wanted-refs delim-pkt] packfile flush-pkt
> +[wanted-refs delim-pkt] [packfile-uris delim-pkt]
> +packfile flush-pkt
While this is an RFC and incomplete, we'd need to remember to
add packfile-uris to the capabilities list above, stating that it requires
thin-pack and ofs-delta to be sent, and what to expect from it.
The mention of --no-packfile-urls in the Client design above
seems to imply we'd want to turn it on by default, which I thought
was not the usual stance how we introduce new things.
An odd way of disabling it would be --no-thin-pack, hoping the
client side implementation abides by the implied requirements.
> acknowledgments = PKT-LINE("acknowledgments" LF)
> (nak | *ack)
> @@ -331,6 +332,9 @@ header. Most sections are sent only when the packfile is
> sent.
> *PKT-LINE(wanted-ref LF)
> wanted-ref = obj-id SP refname
>
> +packfile-uris = PKT-LINE("packfile-uris" LF) *packfile-uri
> +packfile-uri = PKT-LINE("uri" SP *%x20-ff LF)
Is the *%x20-ff a fancy way of saying obj-id?
While the server is configured with pairs of (oid URL),
we would not need to send the exact oid to the client
as that is what the client can figure out on its own by reading
the downloaded pack.
Instead we could send an integrity hash (i.e. the packfile
downloaded from "uri" is expected to hash to $oid here)
Thanks,
Stefan
Re: [WIP RFC 0/5] Design for offloading part of packfile response to CDN
On Mon, Dec 3, 2018 at 3:37 PM Jonathan Tan wrote: > > There is a potential issue: a server which produces both the URIs and > the packfile at roughly the same time (like the implementation in this > patch set) will not have sideband access until it has concluded sending > the URIs. Among other things, this means that the server cannot send > keepalive packets until quite late in the response. One solution to this > might be to add a feature that allows the server to use a sideband > throughout the whole response - and this has other benefits too like > allowing servers to inform the client throughout the whole fetch, not > just at the end. While side band sounds like the right thing to do, we could also sending (NULL)-URIs within this feature.
Re: [PATCH] pack-protocol.txt: accept error packets in any context
> diff --git a/pkt-line.c b/pkt-line.c
> index 04d10bbd0..ce9e42d10 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd,
> char **src_buffer,
> return PACKET_READ_EOF;
> }
>
> + if (starts_with(buffer, "ERR ")) {
> + die(_("remote error: %s"), buffer + 4);
> + }
> +
Handling any ERR line in the pkt reader is okay, as
* we do not pkt-ize pack files and
* we do not have any other parts of the protocol where user data is in
the first four bytes, which could randomly match this pattern and
* the rest of the pkt-ized part of the protocol never sends
ERR lines.
Makes sense.
Re: [PATCH] sideband: color lines with keyword only
Stefan Beller wrote: > On Mon, Dec 3, 2018 at 3:23 PM Jonathan Nieder wrote: >> I was curious about what versions of Gerrit this is designed to >> support (or in other words whether it's a bug fix or a feature). >> Looking at examples like [1], it seems that Gerrit historically always >> used "ERROR:" so the 59a255aef0 logic would work for it. More >> recently, [2] (ReceiveCommits: add a "SUCCESS" marker for successful >> change updates, 2018-08-21) put SUCCESS on a line of its own. That >> puts this squarely in the new-feature category. > > Ooops. From the internal bug, I assumed this to be long standing Gerrit > behavior, which is why I sent it out in -rc to begin with. No worries. Can't hurt for Junio to have a few patches to apply to "pu" or "next" to practice using the release candidates. :) [...] >> In the old code, we would escape early if 'n == len', but we didn't >> need to. If 'n == len', then >> >> src[len] == '\0' > > src[len] could also be one of "\n\r", see the caller > recv_sideband for sidebase case 2. Yes, I noticed too late[*]. Sorry for the noise. The patch still looks good. Jonathan [*] https://public-inbox.org/git/20181203233439.gb157...@google.com/
Re: [PATCH] sideband: color lines with keyword only
On Mon, Dec 3, 2018 at 3:23 PM Jonathan Nieder wrote:
> I was curious about what versions of Gerrit this is designed to
> support (or in other words whether it's a bug fix or a feature).
> Looking at examples like [1], it seems that Gerrit historically always
> used "ERROR:" so the 59a255aef0 logic would work for it. More
> recently, [2] (ReceiveCommits: add a "SUCCESS" marker for successful
> change updates, 2018-08-21) put SUCCESS on a line of its own. That
> puts this squarely in the new-feature category.
Ooops. From the internal bug, I assumed this to be long standing Gerrit
behavior, which is why I sent it out in -rc to begin with.
> > --- a/sideband.c
> > +++ b/sideband.c
> > @@ -87,7 +87,7 @@ static void maybe_colorize_sideband(struct strbuf *dest,
> > const char *src, int n)
> > struct keyword_entry *p = keywords + i;
> > int len = strlen(p->keyword);
> >
> > - if (n <= len)
> > + if (n < len)
> > continue;
>
> In the old code, we would escape early if 'n == len', but we didn't
> need to. If 'n == len', then
>
> src[len] == '\0'
src[len] could also be one of "\n\r", see the caller
recv_sideband for sidebase case 2.
> src .. [len-1] is a valid buffer to read from
>
> so the strncasecmp and strbuf_add operations used in this function are
> valid. Good.
Yes, they are all valid...
> > - if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len]))
> > {
> > + if (!strncasecmp(p->keyword, src, len) &&
> > + (len == n || !isalnum(src[len]))) {
>
> Our custom isalnum treats '\0' as not alphanumeric (sane_ctype[0] ==
> GIT_CNTRL) so this part of the patch is unnecessary. That said, it's
> good for clarity and defensive programming.
... but here we need to check for src[len] for validity.
I made no assumptions about isalnum, but rather needed to shortcut
the condition, as accessing src[len] would be out of bounds, no?
>
> > strbuf_addstr(dest, p->color);
> > strbuf_add(dest, src, len);
unlike here (or the rest of the block), where len is used correctly.
Re: [PATCH] sideband: color lines with keyword only
Jonathan Nieder wrote:
> Stefan Beller wrote:
>> /*
>> * Match case insensitively, so we colorize output from existing
>> @@ -95,7 +95,8 @@ static void maybe_colorize_sideband(struct strbuf *dest,
>> const char *src, int n)
>> * messages. We only highlight the word precisely, so
>> * "successful" stays uncolored.
>> */
>> -if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {
>> +if (!strncasecmp(p->keyword, src, len) &&
>> +(len == n || !isalnum(src[len]))) {
>
> Our custom isalnum treats '\0' as not alphanumeric (sane_ctype[0] ==
> GIT_CNTRL) so this part of the patch is unnecessary. That said, it's
> good for clarity and defensive programming.
Correction: I am being silly here. src[len] can be '\0', '\n', or
'\r' --- it's not always '\0'. And the contract of this function is
that src[len] could be anything. Thanks for having handled it
correctly. :)
Jonathan
Re: [PATCH] sideband: color lines with keyword only
Hi,
Stefan Beller wrote:
> When bf1a11f0a1 (sideband: highlight keywords in remote sideband output,
> 2018-08-07) was introduced, it was carefully considered which strings
> would be highlighted. However 59a255aef0 (sideband: do not read beyond
> the end of input, 2018-08-18) brought in a regression that the original
> did not test for. A line containing only the keyword and nothing else
> ("SUCCESS") should still be colored.
>
> Signed-off-by: Stefan Beller
> ---
> sideband.c | 5 +++--
> t/t5409-colorize-remote-messages.sh | 2 ++
> 2 files changed, 5 insertions(+), 2 deletions(-)
Thanks for writing this.
I was curious about what versions of Gerrit this is designed to
support (or in other words whether it's a bug fix or a feature).
Looking at examples like [1], it seems that Gerrit historically always
used "ERROR:" so the 59a255aef0 logic would work for it. More
recently, [2] (ReceiveCommits: add a "SUCCESS" marker for successful
change updates, 2018-08-21) put SUCCESS on a line of its own. That
puts this squarely in the new-feature category.
"success" on its own line is even less likely to be a false positive
than "success" followed by punctuation (for example a period marking
the end of a sentence). So I like this change.
[1] https://gerrit-review.googlesource.com/c/gerrit/+/22361
[2] https://gerrit-review.googlesource.com/c/gerrit/+/193570
> diff --git a/sideband.c b/sideband.c
> index 368647acf8..7c3d33d3f8 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -87,7 +87,7 @@ static void maybe_colorize_sideband(struct strbuf *dest,
> const char *src, int n)
> struct keyword_entry *p = keywords + i;
> int len = strlen(p->keyword);
>
> - if (n <= len)
> + if (n < len)
> continue;
In the old code, we would escape early if 'n == len', but we didn't
need to. If 'n == len', then
src[len] == '\0'
src .. [len-1] is a valid buffer to read from
so the strncasecmp and strbuf_add operations used in this function are
valid. Good.
> /*
>* Match case insensitively, so we colorize output from existing
> @@ -95,7 +95,8 @@ static void maybe_colorize_sideband(struct strbuf *dest,
> const char *src, int n)
>* messages. We only highlight the word precisely, so
>* "successful" stays uncolored.
>*/
> - if (!strncasecmp(p->keyword, src, len) && !isalnum(src[len])) {
> + if (!strncasecmp(p->keyword, src, len) &&
> + (len == n || !isalnum(src[len]))) {
Our custom isalnum treats '\0' as not alphanumeric (sane_ctype[0] ==
GIT_CNTRL) so this part of the patch is unnecessary. That said, it's
good for clarity and defensive programming.
> strbuf_addstr(dest, p->color);
> strbuf_add(dest, src, len);
> strbuf_addstr(dest, GIT_COLOR_RESET);
> diff --git a/t/t5409-colorize-remote-messages.sh
> b/t/t5409-colorize-remote-messages.sh
> index f81b6813c0..2a8c449661 100755
> --- a/t/t5409-colorize-remote-messages.sh
> +++ b/t/t5409-colorize-remote-messages.sh
> @@ -17,6 +17,7 @@ test_expect_success 'setup' '
> echo " " "error: leading space"
> echo ""
> echo Err
> + echo SUCCESS
> exit 0
> EOF
> echo 1 >file &&
> @@ -35,6 +36,7 @@ test_expect_success 'keywords' '
> grep "error: error" decoded &&
> grep "hint:" decoded &&
> grep "success:" decoded &&
> + grep "SUCCESS" decoded &&
> grep "warning:" decoded
> '
Nice tests.
The "hinting: not highlighted" example shows that we aren't
introducing false positives here, so the coverage seems sufficient.
It might be nice to include a line
echo ERROR:
as well to match another idiom that Gerrit sometimes uses.
Reviewed-by: Jonathan Nieder
Thanks again for a pleasant read.
Re: easy way to demonstrate length of colliding SHA-1 prefixes?
On Mon, Dec 03, 2018 at 02:30:44PM -0800, Matthew DeVore wrote:
> Here is a one-liner to do it. It is Perl line noise, so it's not very cute,
> thought that is subjective. The output shown below is for the Git project
> (not Linux) repository as I've currently synced it:
>
> $ git rev-list --objects HEAD | sort | perl -anE 'BEGIN { $prev = ""; $long
> = "" } $n = $F[0]; for my $i (reverse 1..40) {last if $i < length($long); if
> (substr($prev, 0, $i) eq substr($n, 0, $i)) {$long = substr($prev, 0, $i);
> last} } $prev = $n; END {say $long}'
Ooh, object-collision golf.
Try:
git cat-file --batch-all-objects --batch-check='%(objectname)'
instead of "rev-list | sort". It's _much_ faster, because it doesn't
have to actually open the objects and walk the graph.
Some versions of uniq have "-w" (including GNU, but it's definitely not
in POSIX), which lets you do:
git cat-file --batch-all-objects --batch-check='%(objectname)' |
uniq -cdw 7
to list all collisions of length 7 (it will show just the first item
from each group, but you can use -D to see them all).
> > You'll always need to list them all. It's inherently an operation where
> > for each SHA-1 you need to search for other ones with that prefix up to
> > a given length.
> >
> > Perhaps you've missed that you can use --abbrev=N for this, and just
> > grep for things that are loger than that N, e.g. for linux.git:
> >
> > git log --oneline --abbrev=10 --pretty=format:%h |
> > grep -E -v '^.{10}$' |
> > perl -pe 's/^(.{10}).*/$1/'
>
> I think the goal was to search all object hashes, not just commits. And git
> rev-list --objects will do that.
You can add "-t --raw" to see the abbreviated tree and blob names,
though it gets tricky around handling merges.
-Peff
Re: easy way to demonstrate length of colliding SHA-1 prefixes?
On 12/02/2018 05:23 AM, Ævar Arnfjörð Bjarmason wrote:
On Sun, Dec 02 2018, Robert P. J. Day wrote:
as part of an upcoming git class i'm delivering, i thought it would
be amusing to demonstrate the maximum length of colliding SHA-1
prefixes in a repository (in my case, i use the linux kernel git repo
for most of my examples).
is there a way to display the objects in the object database that
clash in the longest object name SHA-1 prefix; i mean, short of
manually listing all object names, running that through cut and sort
and uniq and ... you get the idea.
is there a cute way to do that? thanks.
Here is a one-liner to do it. It is Perl line noise, so it's not very
cute, thought that is subjective. The output shown below is for the Git
project (not Linux) repository as I've currently synced it:
$ git rev-list --objects HEAD | sort | perl -anE 'BEGIN { $prev = "";
$long = "" } $n = $F[0]; for my $i (reverse 1..40) {last if $i <
length($long); if (substr($prev, 0, $i) eq substr($n, 0, $i)) {$long =
substr($prev, 0, $i); last} } $prev = $n; END {say $long}'
c68038ef
$ git cat-file -t c68038ef
error: short SHA1 c68038ef is ambiguous
hint: The candidates are:
hint: c68038effe commit 2012-06-01 - vcs-svn: suppress a
signed/unsigned comparison warning
hint: c68038ef00 blob
fatal: Not a valid object name c68038ef
You'll always need to list them all. It's inherently an operation where
for each SHA-1 you need to search for other ones with that prefix up to
a given length.
Perhaps you've missed that you can use --abbrev=N for this, and just
grep for things that are loger than that N, e.g. for linux.git:
git log --oneline --abbrev=10 --pretty=format:%h |
grep -E -v '^.{10}$' |
perl -pe 's/^(.{10}).*/$1/'
I think the goal was to search all object hashes, not just commits. And
git rev-list --objects will do that.
Re: [PATCH v2] revisions.c: put promisor option in specialized struct
On Mon, Dec 03, 2018 at 02:10:19PM -0800, Matthew DeVore wrote: > Put the allow_exclude_promisor_objects flag in setup_revision_opt. When > it was in rev_info, it was unclear when it was used, since rev_info is > passed to functions that don't use the flag. This resulted in > unnecessary setting of the flag in prune.c, so fix that as well. > > Signed-off-by: Matthew DeVore > --- > builtin/pack-objects.c | 6 -- > builtin/prune.c| 1 - > builtin/rev-list.c | 6 -- > revision.c | 10 ++ > revision.h | 4 ++-- > 5 files changed, 16 insertions(+), 11 deletions(-) Thanks, this version looks good to me. One style nit that I don't think is worth a re-roll, but that Junio might want to tweak while applying: > diff --git a/revision.c b/revision.c > index 13e0519c02..f6b32e6a42 100644 > --- a/revision.c > +++ b/revision.c > @@ -1791,7 +1791,8 @@ static void add_message_grep(struct rev_info *revs, > const char *pattern) > } > > static int handle_revision_opt(struct rev_info *revs, int argc, const char > **argv, > -int *unkc, const char **unkv) > +int *unkc, const char **unkv, > +const struct setup_revision_opt* opt) We keep the "*" with the variable name, not the type. -Peff
Re: Confusing inconsistent option syntax
On Sun, Dec 02, 2018 at 09:07:47PM +1100, Robert White wrote: > `git log --pretty short` gives the error message "ambiguous argument > 'short'". To get the expected result, you need to use `git log > --pretty=short`. However, `git log --since yesterday` and `git log > --since=yesterday` both work as expected. > > When is an = needed? What is the reason for these inconsistencies? As Duy mentioned, this has to do with optional arguments for some flags. This is discussed in more detail in "git help cli". Notably (and as advised in that manpage), you should always use the "stuck" form (with the "=") in scripts, in case a flag grows an optional value later. -Peff
Re: [PATCH] revisions.c: put promisor option in specialized struct
On 12/03/2018 01:24 PM, Jeff King wrote:
@@ -297,7 +296,8 @@ struct setup_revision_opt {
const char *def;
void (*tweak)(struct rev_info *, struct setup_revision_opt *);
const char *submodule; /* TODO: drop this and use rev_info->repo */
- int assume_dashdash;
+ int assume_dashdash : 1;
+ int allow_exclude_promisor_objects : 1;
unsigned revarg_opt;
};
I don't know that we need to penny-pinch bytes in this struct, but in
general it shouldn't hurt either awy. However, a signed bit-field with 1
bit is funny. I'm not even sure what the standard has to say, but in
twos-complement that would store "-1" and "0" (gcc -Wpedantic also
complains about overflow in assigning "1" to it).
Interesting. I hadn't suspected this. But I confirmed it with this:
#include
struct x {
int y : 1;
int z : 1;
};
int main() {
struct x x;
x.y = 1;
x.z = 1;
printf("%d %d\n", (int) x.y, (int) x.z);
return 0;
}
-- Output --
-1 -1
So this probably ought to be "unsigned".
Earlier in this file we define bit fields this way:
/* Traversal flags */
unsigned intdense:1,
prune:1,
... using \t to align the field names, so I'll mimic that style.
Re: [PATCH 8/9] sha1-file: use loose object cache for quick existence check
On Sun, Dec 02, 2018 at 11:52:50AM +0100, René Scharfe wrote:
> > And for mu.git, a ~20k object repo:
> >
> > Test origin/master
> > peff/jk/loose-cache avar/check-collisions-config
> >
> > -
> > 0008.2: index-pack with 256*1 loose objects 0.59(0.91+0.06)
> > 0.58(0.93+0.03) -1.7% 0.57(0.89+0.04) -3.4%
> > 0008.3: index-pack with 256*10 loose objects 0.59(0.91+0.07)
> > 0.59(0.92+0.03) +0.0% 0.57(0.89+0.03) -3.4%
> > 0008.4: index-pack with 256*100 loose objects0.59(0.91+0.05)
> > 0.81(1.13+0.04) +37.3%0.58(0.91+0.04) -1.7%
> > 0008.5: index-pack with 256*250 loose objects0.59(0.91+0.05)
> > 1.23(1.51+0.08) +108.5% 0.58(0.91+0.04) -1.7%
> > 0008.6: index-pack with 256*500 loose objects0.59(0.90+0.06)
> > 1.96(2.20+0.12) +232.2% 0.58(0.91+0.04) -1.7%
> > 0008.7: index-pack with 256*750 loose objects0.59(0.92+0.05)
> > 2.72(2.92+0.17) +361.0% 0.58(0.90+0.04) -1.7%
> > 0008.8: index-pack with 256*1000 loose objects 0.59(0.90+0.06)
> > 3.50(3.67+0.21) +493.2% 0.57(0.90+0.04) -3.4%
>
> OK, here's another theory: The cache scales badly with increasing
> numbers of loose objects because it sorts the array 256 times as it is
> filled. Loading it fully and sorting once would help, as would using
> one array per subdirectory.
Yeah, that makes sense. This was actually how I had planned to do it
originally, but then I ended up just reusing the existing single-array
approach from the abbrev code.
I hadn't actually thought about the repeated sortings (but that
definitely makes sense that they would hurt in these pathological
cases), but more just that we get a 256x reduction in N for our binary
search (in fact we already do this first-byte lookup-table trick for
pack index lookups).
Your patch looks good to me. We may want to do one thing on top:
> diff --git a/object-store.h b/object-store.h
> index 8dceed0f31..ee67a50980 100644
> --- a/object-store.h
> +++ b/object-store.h
> @@ -20,7 +20,7 @@ struct object_directory {
>* Be sure to call odb_load_loose_cache() before using.
>*/
> char loose_objects_subdir_seen[256];
> - struct oid_array loose_objects_cache;
> + struct oid_array loose_objects_cache[256];
The comment in the context there is warning callers to remember to load
the cache first. Now that we have individual caches, might it make sense
to change the interface a bit, and make these members private. I.e.,
something like:
struct oid_array *odb_loose_cache(struct object_directory *odb,
int subdir_nr)
{
if (!loose_objects_subdir_seen[subdir_nr])
odb_load_loose_cache(odb, subdir_nr); /* or just inline it here
*/
return >loose_objects_cache[subdir_nr];
}
That's harder to get wrong, and this:
> diff --git a/sha1-file.c b/sha1-file.c
> index 05f63dfd4e..d2f5e65865 100644
> --- a/sha1-file.c
> +++ b/sha1-file.c
> @@ -933,7 +933,8 @@ static int quick_has_loose(struct repository *r,
> prepare_alt_odb(r);
> for (odb = r->objects->odb; odb; odb = odb->next) {
> odb_load_loose_cache(odb, subdir_nr);
> - if (oid_array_lookup(>loose_objects_cache, ) >= 0)
> + if (oid_array_lookup(>loose_objects_cache[subdir_nr],
> + ) >= 0)
> return 1;
> }
becomes:
struct oid_array *cache = odb_loose_cache(odb, subdir_nr);
if (oid_array_lookup(cache, ))
return 1;
(An even simpler interface would be a single function that computes
subdir_nr and does the lookup itself, but that would not be enough for
find_short_object_filename()).
-Peff
Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed
On 12/03/2018 01:15 PM, Jeff King wrote:
That said, our C99 designated initializer weather-balloons haven't
gotten any complaints yet. So I think you could actually do:
struct setup_revision_opt s_r_opt = {
.allow_exclude_promisor_objects = 1,
};
I like this way best, so I'll use it. Thank you.
Re: [PATCH/RFC v2 0/7] Introduce new commands switch-branch and checkout-files
On Thu, Nov 29, 2018 at 7:33 AM Duy Nguyen wrote:
>
> On Wed, Nov 28, 2018 at 9:30 PM Stefan Beller wrote:
> >
> > On Wed, Nov 28, 2018 at 12:09 PM Duy Nguyen wrote:
> > >
> > > On Wed, Nov 28, 2018 at 9:01 PM Duy Nguyen wrote:
> > > > should we do
> > > > something about detached HEAD in this switch-branch command (or
> > > > whatever its name will be)?
> > > >
> > > > This is usually a confusing concept to new users
> > >
> > > And it just occurred to me that perhaps we should call this "unnamed
> > > branch" (at least at high UI level) instead of detached HEAD. It is
> > > technically not as accurate, but much better to understand.
> >
> > or 'direct' branch?
>
> makes me think, what is an indirect branch?
I drew the term from HEAD pointing to a branch pointing
to a commit, i.e. HEAD indirectly points to a commit, but
in 'direct' branch mode, HEAD contains the commit id.
So indirect branch would work for our current 'real' branches.
When asked out of context of this discussion, I might add
yet another layer of abstraction to make an 'indirect branch',
i.e. HEAD pointing to a symbolic ref that points at a branch
that points to a commit.
The term symref is what we currently use
(Just looked into gitglossary, where we distinguish
symbolic refs from pseudorefs) for hat I would have called
an indirect branch as well.
So maybe we need to measure the level of indirection
("How often do we need to dereference the ref/object to get
a commit oid?") to come to terms in how to describe
the use cases easily.
Here is a fun-one:
git checkout
git checkout --detach
Currently the --detach option detaches HEAD from
branch pointing at the object id, i.e. it is the same as
git checkout
whereas when we focus on the levels of indirection
it would also be reasonable to have
git checkout
as a reasonable alternative, where is the
branch that is pointed at from the .
Re: [PATCH] rebase -i: introduce the 'test' command
On Mon, Dec 03, 2018 at 06:53:22PM +0100, Duy Nguyen wrote: > On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote: > > I sometimes add "x false" to the top of the todo list to stop and create > > new commits before the first one. > > And here I've been doing the same by "edit" the first commit, add a > new commit then reorder them in the second interactive rebase :P > > This made me look at git-rebase.txt to really learn about interactive > rebase. I think the interactive rebase section could use some > improvements. Its style looks.. umm.. more story telling than a > reference. Perhaps something like this to at least highlight the > commands. Yeah, I think the typographic change is an improvement that doesn't really have a downside. As Dscho mentioned, sometimes the story style is what you want, so I don't think we'd want to simply rearrange it. It may be useful to present the material twice, though: once as a table/list for reference, and then once as a story working through an example. -Peff
Re: [PATCH] rebase -i: introduce the 'test' command
On Mon, Dec 03, 2018 at 08:01:44PM +0100, Johannes Schindelin wrote: > > In this sort of situation, I often whish to be able to do nested rebases. > > Even more because it happen relatively often that I forget that I'm > > working in a rebase and not on the head, and then it's quite natural > > to me to type things like 'git rebase -i @^^^' while already rebasing. > > But I suppose this has already been discussed. > > Varieties of this have been discussed, but no, not nested rebases. > > The closest we thought about was re-scheduling the latest commits, > which is now harder because of the `--rebase-merges` mode. > > But I think it would be doable. Your idea of a "nested" rebase actually > opens that door quite nicely. It would not *really* be a nested rebase, > and it would still only be possible in interactive mode, but I could > totally see > > git rebase --nested -i HEAD~3 > > to generate and prepend the following lines to the `git-rebase-todo` file: > > reset abcdef01 # This is HEAD~3 > pick abcdef02 # This is HEAD~2 > pick abcdef03 # This is HEAD~ > pick abcdef04 # This is HEAD > > (assuming that the latest 3 commits were non-merge commits; It would look > quite a bit more complicated in other situations.) Yeah, I would probably use that if it existed. It would be nicer to have real nested sequencer operations, I think, for other situations. E.g., cherry-picking a sequence of commits while you're in the middle of a rebase. But I suspect getting that right would be _loads_ more work, and probably would involve some funky UI corner cases to handle the stack of operations (so truly aborting a rebase may mean an arbitrary number of "rebase --abort" calls to pop the stack). Your suggestion is probably a reasonable trick in the meantime. -Peff
Re: [PATCH] rebase -i: introduce the 'test' command
On Mon, Dec 03, 2018 at 02:31:37PM +, Phillip Wood wrote: > > How would I move past the test that fails to continue? I guess "git > > rebase --edit-todo" and then manually remove it (and any other remaining > > test lines)? > > Perhaps we could teach git rebase --skip to skip a rescheduled command, it > could be useful if people want to skip rescheduled picks as well (though I > don't think I've ever had that happen in the wild). I can see myself turning > on the rescheduling config setting but occasionally wanting to be able to > skip over the rescheduled exec command. Yeah, I agree that would give a nice user experience. -Peff
Re: [PATCH] revisions.c: put promisor option in specialized struct
On Mon, Dec 03, 2018 at 11:23:56AM -0800, Matthew DeVore wrote:
> Put the allow_exclude_promisor_objects flag in setup_revision_opt. When
> it was in rev_info, it was unclear when it was used, since rev_info is
> passed to functions that don't use the flag. This resulted in
> unnecessary setting of the flag in prune.c, so fix that as well.
>
> Signed-off-by: Matthew DeVore
> ---
> builtin/pack-objects.c | 7 +--
> builtin/prune.c| 1 -
> builtin/rev-list.c | 6 --
> revision.c | 10 ++
> revision.h | 4 ++--
> 5 files changed, 17 insertions(+), 11 deletions(-)
Thanks, this mostly looks good to me (with or without tweaking the
initializers as discussed in the other part of the thread).
One thing I noticed:
> @@ -297,7 +296,8 @@ struct setup_revision_opt {
> const char *def;
> void (*tweak)(struct rev_info *, struct setup_revision_opt *);
> const char *submodule; /* TODO: drop this and use rev_info->repo */
> - int assume_dashdash;
> + int assume_dashdash : 1;
> + int allow_exclude_promisor_objects : 1;
> unsigned revarg_opt;
> };
I don't know that we need to penny-pinch bytes in this struct, but in
general it shouldn't hurt either awy. However, a signed bit-field with 1
bit is funny. I'm not even sure what the standard has to say, but in
twos-complement that would store "-1" and "0" (gcc -Wpedantic also
complains about overflow in assigning "1" to it).
So this probably ought to be "unsigned".
-Peff
Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed
On Mon, Dec 03, 2018 at 11:10:49AM -0800, Matthew DeVore wrote:
> > > + memset(_r_opt, 0, sizeof(s_r_opt));
> > > + s_r_opt.allow_exclude_promisor_objects = 1;
> > > + setup_revisions(ac, av, , _r_opt);
> >
> > I wonder if a static initializer for setup_revision_opt is worth it. It
> > would remove the need for this memset. Probably not a big deal either
> > way, though.
> I think you mean something like this:
>
> static struct setup_revision_opt s_r_opt = {NULL, NULL, NULL, 0, 1, 0};
>
> This is a bit cryptic (I have to read the struct declaration in order to
> know what is being set to 1) and if the struct ever gets a new field before
> allow_exclude_promisor_objects, this initializer has to be updated.
I agree that's pretty awful. I meant something like this:
struct setup_revision_opt s_r_opt = { NULL };
...
s_r_opt.allow_exclude_promisor_objects = 1;
setup_revisions(...);
It's functionally equivalent to the memset(), but you don't have to
wonder about whether we peek at the uninitialized state in between.
That said, our C99 designated initializer weather-balloons haven't
gotten any complaints yet. So I think you could actually do:
struct setup_revision_opt s_r_opt = {
.allow_exclude_promisor_objects = 1,
};
...
setup_revisions(...);
which is pretty nice.
-Peff
Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences
Hi Hannes, On Mon, 3 Dec 2018, Johannes Sixt wrote: > The text body of section Behavioral Differences is typeset as code, > but should be regular text. Remove the indentation to achieve that. > > While here, prettify the language: > > - use "the x backend" instead of "x-based rebase"; > - use present tense instead of future tense; > > and use subsections instead of a list. > > Signed-off-by: Johannes Sixt > --- The changes look sensible to me. Thanks, Dscho > Cf. https://git-scm.com/docs/git-rebase#_behavioral_differences > > I actually did not test the result, because I don't have the > infrastructure. > > The sentence "The am backend sometimes does not" is not very useful, > but is not my fault ;) It would be great if it could be made more > specific, but I do not know the details. > > Documentation/git-rebase.txt | 30 +- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 80793bad8d..dff17b3178 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -550,24 +550,28 @@ Other incompatible flag pairs: > BEHAVIORAL DIFFERENCES > --- > > - * empty commits: > +There are some subtle differences how the backends behave. > > -am-based rebase will drop any "empty" commits, whether the > -commit started empty (had no changes relative to its parent to > -start with) or ended empty (all changes were already applied > -upstream in other commits). > +Empty commits > +~ > + > +The am backend drops any "empty" commits, regardless of whether the > +commit started empty (had no changes relative to its parent to > +start with) or ended empty (all changes were already applied > +upstream in other commits). > > -merge-based rebase does the same. > +The merge backend does the same. > > -interactive-based rebase will by default drop commits that > -started empty and halt if it hits a commit that ended up empty. > -The `--keep-empty` option exists for interactive rebases to allow > -it to keep commits that started empty. > +The interactive backend drops commits by default that > +started empty and halts if it hits a commit that ended up empty. > +The `--keep-empty` option exists for the interactive backend to allow > +it to keep commits that started empty. > > - * directory rename detection: > +Directory rename detection > +~~ > > -merge-based and interactive-based rebases work fine with > -directory rename detection. am-based rebases sometimes do not. > +The merge and interactive backends work fine with > +directory rename detection. The am backend sometimes does not. > > include::merge-strategies.txt[] > > -- > 2.19.1.1133.g2dd3d172d2 >
Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences
Am 03.12.18 um 21:42 schrieb Martin Ågren: On Mon, 3 Dec 2018 at 18:35, Johannes Sixt wrote: I actually did not test the result, because I don't have the infrastructure. I've tested with asciidoc and Asciidoctor, html and man-page. Looks good. Thank you so much! -- Hannes
Re: [ANNOUNCE] Git v2.20.0-rc2
een corrected.
>(merge e0a862fdaf sb/submodule-url-to-absolute later to maint).
>
> * "git fetch" over protocol v2 into a shallow repository failed to
>fetch full history behind a new tip of history that was diverged
>before the cut-off point of the history that was previously fetched
>shallowly.
>
> * The command line completion machinery (in contrib/) has been
>updated to allow the completion script to tweak the list of options
>that are reported by the parse-options machinery correctly.
>(merge 276b49ff34 nd/completion-negation later to maint).
>
> * Operations on promisor objects make sense in the context of only a
>small subset of the commands that internally use the revisions
>machinery, but the "--exclude-promisor-objects" option were taken
>and led to nonsense results by commands like "log", to which it
>didn't make much sense. This has been corrected.
>(merge 669b1d2aae md/exclude-promisor-objects-fix later to maint).
>
> * The "container" mode of TravisCI is going away. Our .travis.yml
>file is getting prepared for the transition.
>(merge 32ee384be8 ss/travis-ci-force-vm-mode later to maint).
>
> * Our test scripts can now take the '-V' option as a synonym for the
>'--verbose-log' option.
>(merge a5f52c6dab sg/test-verbose-log later to maint).
>
> * A regression in Git 2.12 era made "git fsck" fall into an infinite
>loop while processing truncated loose objects.
>(merge 18ad13e5b2 jk/detect-truncated-zlib-input later to maint).
>
> * "git ls-remote $there foo" was broken by recent update for the
>protocol v2 and stopped showing refs that match 'foo' that are not
>refs/{heads,tags}/foo, which has been fixed.
>(merge 6a139cdd74 jk/proto-v2-ref-prefix-fix later to maint).
>
> * Additional comment on a tricky piece of code to help developers.
>(merge 0afbe3e806 jk/stream-pack-non-delta-clarification later to maint).
>
> * A couple of tests used to leave the repository in a state that is
>deliberately corrupt, which have been corrected.
>(merge aa984dbe5e ab/pack-tests-cleanup later to maint).
>
> * The submodule support has been updated to read from the blob at
>HEAD:.gitmodules when the .gitmodules file is missing from the
>working tree.
>(merge 2b1257e463 ao/submodule-wo-gitmodules-checked-out later to maint).
>
> * "git fetch" was a bit loose in parsing responses from the other side
>when talking over the protocol v2.
>
> * "git rev-parse --exclude=* --branches --branches" (i.e. first
>saying "add only things that do not match '*' out of all branches"
>and then adding all branches, without any exclusion this time")
>worked as expected, but "--exclude=* --all --all" did not work the
>same way, which has been fixed.
>(merge 5221048092 ag/rev-parse-all-exclude-fix later to maint).
>
> * "git send-email --transfer-encoding=..." in recent versions of Git
>sometimes produced an empty "Content-Transfer-Encoding:" header,
>which has been corrected.
>(merge 3c88e46f1a al/send-email-auto-cte-fixup later to maint).
>
> * The interface into "xdiff" library used to discover the offset and
>size of a generated patch hunk by first formatting it into the
>textual hunk header "@@ -n,m +k,l @@" and then parsing the numbers
>out. A new interface has been introduced to allow callers a more
>direct access to them.
>(merge 5eade0746e jk/xdiff-interface later to maint).
>
> * Pathspec matching against a tree object were buggy when negative
>pathspec elements were involved, which has been fixed.
>(merge b7845cebc0 nd/tree-walk-path-exclusion later to maint).
>
> * "git merge" and "git pull" that merges into an unborn branch used
>to completely ignore "--verify-signatures", which has been
>corrected.
>(merge 01a31f3bca jk/verify-sig-merge-into-void later to maint).
>
> * "git rebase --autostash" did not correctly re-attach the HEAD at times.
>
> * "rev-parse --exclude= --branches=" etc. did not
>quite work, which has been corrected.
>(merge 9ab9b5df0e ra/rev-parse-exclude-glob later to maint).
>
> * When editing a patch in a "git add -i" session, a hunk could be
>made to no-op. The "git apply" program used to reject a patch with
>such a no-op hunk to catch user mistakes, but it is now updated to
>explicitly allow a no-op hunk in an edited patch.
>(merge 22cb3835b9 js/apply-recount-allow-noop later to maint
Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences
On Mon, 3 Dec 2018 at 18:35, Johannes Sixt wrote: > I actually did not test the result, because I don't have the > infrastructure. I've tested with asciidoc and Asciidoctor, html and man-page. Looks good. Martin
[PATCH 0/3] Re: [ANNOUNCE] Git v2.20.0-rc2
Hi Junio, > A release candidate Git v2.20.0-rc2 is now available for testing > at the usual places. It is comprised of 934 non-merge commits > since v2.19.0, contributed by 76 people, 25 of which are new faces. Here are a few suggested tweaks after reading the draft release notes. Nothing critical. Martin Martin Ågren (3): RelNotes 2.20: move some items between sections RelNotes 2.20: clarify sentence RelNotes 2.20: drop spurious double quote Documentation/RelNotes/2.20.0.txt | 30 +++--- 1 file changed, 15 insertions(+), 15 deletions(-) -- 2.20.0.rc2.1.gfcc5f94f1e
Re: [PATCH] rebase -i: introduce the 'test' command
On Mon, Dec 03, 2018 at 08:01:44PM +0100, Johannes Schindelin wrote: > Hi Luc, > > On Mon, 3 Dec 2018, Luc Van Oostenryck wrote: > > > On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote: > > > 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. > > > > > > I wonder how often this kind of "yes, I know it fails, but keep going > > > anyway" situation would come up. And what the interface is like for > > > getting past it. E.g., what if you fixed a bunch of stuff but your tests > > > still fail? You may not want to abandon the changes you've made, but you > > > need to "rebase --continue" to move forward. I encounter this often when > > > the correct fix is actually in an earlier commit than the one that > > > yields the test failure. You can't rewind an interactive rebase, so I > > > complete and restart it, adding an "e"dit at the earlier commit. > > > > In this sort of situation, I often whish to be able to do nested rebases. > > Even more because it happen relatively often that I forget that I'm > > working in a rebase and not on the head, and then it's quite natural > > to me to type things like 'git rebase -i @^^^' while already rebasing. > > But I suppose this has already been discussed. > > Varieties of this have been discussed, but no, not nested rebases. Interesting :) > The closest we thought about was re-scheduling the latest commits, > which is now harder because of the `--rebase-merges` mode. > > But I think it would be doable. Your idea of a "nested" rebase actually > opens that door quite nicely. It would not *really* be a nested rebase, > and it would still only be possible in interactive mode, but I could > totally see > > git rebase --nested -i HEAD~3 I don't mind much if it would be "really nested" or "as-if nested" but with this flag --nested I wonder what would happen if I would use it in a 'top-level' rebase (or, said in another way, would I be able to alias 'rebase' to 'rebase --nested')? > to generate and prepend the following lines to the `git-rebase-todo` file: > > reset abcdef01 # This is HEAD~3 > pick abcdef02 # This is HEAD~2 > pick abcdef03 # This is HEAD~ > pick abcdef04 # This is HEAD > OK, I see. This would not be nestable/stackable but would solve the problem nicely. Best regards, -- Luc
Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed
On 12/01/2018 11:44 AM, Jeff King wrote:
repo_init_revisions(the_repository, , NULL);
save_commit_buffer = 0;
- revs.allow_exclude_promisor_objects_opt = 1;
- setup_revisions(ac, av, , NULL);
+
+ memset(_r_opt, 0, sizeof(s_r_opt));
+ s_r_opt.allow_exclude_promisor_objects = 1;
+ setup_revisions(ac, av, , _r_opt);
I wonder if a static initializer for setup_revision_opt is worth it. It
would remove the need for this memset. Probably not a big deal either
way, though.
I think you mean something like this:
static struct setup_revision_opt s_r_opt = {NULL, NULL, NULL, 0, 1, 0};
This is a bit cryptic (I have to read the struct declaration in order to
know what is being set to 1) and if the struct ever gets a new field
before allow_exclude_promisor_objects, this initializer has to be updated.
static int handle_revision_opt(struct rev_info *revs, int argc, const char
**argv,
- int *unkc, const char **unkv)
+ int *unkc, const char **unkv,
+ int allow_exclude_promisor_objects)
Why not pass in the whole setup_revision_opt struct? We don't need
anything else from it yet, but it seems like the point of that struct is
to pass around preferences like this.
OK, the code reads better if I do that, so I agree.
-Peff
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Duy, On Mon, 3 Dec 2018, Duy Nguyen wrote: > On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote: > > I sometimes add "x false" to the top of the todo list to stop and create > > new commits before the first one. > > And here I've been doing the same by "edit" the first commit, add a > new commit then reorder them in the second interactive rebase :P > > This made me look at git-rebase.txt to really learn about interactive > rebase. I think the interactive rebase section could use some > improvements. Its style looks.. umm.. more story telling than a > reference. Perhaps something like this to at least highlight the > commands. And maybe, just maybe, that "story telling" is more useful for users who want to learn about the interactive rebase, just like yourself, when compared to a mere "reference". Ciao, Johannes
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Luc, On Mon, 3 Dec 2018, Luc Van Oostenryck wrote: > On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote: > > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: > > > > > > > Would it not make more sense to add a command-line option (and a > > > > > config > > > > > setting) to re-schedule failed `exec` commands? Like so: > > > > > > > > Your proposition would do in most cases, however it is not possible to > > > > make a distinction between reschedulable and non-reschedulable commands. > > > > > > True. But I don't think that's so terrible. > > > > > > What I think is something to avoid is two commands that do something very, > > > very similar, but with two very, very different names. > > > > > > 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. > > > > I wonder how often this kind of "yes, I know it fails, but keep going > > anyway" situation would come up. And what the interface is like for > > getting past it. E.g., what if you fixed a bunch of stuff but your tests > > still fail? You may not want to abandon the changes you've made, but you > > need to "rebase --continue" to move forward. I encounter this often when > > the correct fix is actually in an earlier commit than the one that > > yields the test failure. You can't rewind an interactive rebase, so I > > complete and restart it, adding an "e"dit at the earlier commit. > > In this sort of situation, I often whish to be able to do nested rebases. > Even more because it happen relatively often that I forget that I'm > working in a rebase and not on the head, and then it's quite natural > to me to type things like 'git rebase -i @^^^' while already rebasing. > But I suppose this has already been discussed. Varieties of this have been discussed, but no, not nested rebases. The closest we thought about was re-scheduling the latest commits, which is now harder because of the `--rebase-merges` mode. But I think it would be doable. Your idea of a "nested" rebase actually opens that door quite nicely. It would not *really* be a nested rebase, and it would still only be possible in interactive mode, but I could totally see git rebase --nested -i HEAD~3 to generate and prepend the following lines to the `git-rebase-todo` file: reset abcdef01 # This is HEAD~3 pick abcdef02 # This is HEAD~2 pick abcdef03 # This is HEAD~ pick abcdef04 # This is HEAD (assuming that the latest 3 commits were non-merge commits; It would look quite a bit more complicated in other situations.) Ciao, Dscho
Re: [PATCH] rebase -i: introduce the 'test' command
On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote:
> I sometimes add "x false" to the top of the todo list to stop and create
> new commits before the first one.
And here I've been doing the same by "edit" the first commit, add a
new commit then reorder them in the second interactive rebase :P
This made me look at git-rebase.txt to really learn about interactive
rebase. I think the interactive rebase section could use some
improvements. Its style looks.. umm.. more story telling than a
reference. Perhaps something like this to at least highlight the
commands.
-- 8< --
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 80793bad8d..c569b3370b 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -637,22 +637,22 @@ The oneline descriptions are purely for your pleasure;
'git rebase' will
not look at them but at the commit names ("deadbee" and "fa1afe1" in this
example), so do not delete or edit the names.
-By replacing the command "pick" with the command "edit", you can tell
+By replacing the command `pick` with the command `edit`, you can tell
'git rebase' to stop after applying that commit, so that you can edit
the files and/or the commit message, amend the commit, and continue
rebasing.
To interrupt the rebase (just like an "edit" command would do, but without
-cherry-picking any commit first), use the "break" command.
+cherry-picking any commit first), use the `break` command.
If you just want to edit the commit message for a commit, replace the
-command "pick" with the command "reword".
+command "pick" with the command `reword`.
-To drop a commit, replace the command "pick" with "drop", or just
+To drop a commit, replace the command "pick" with `drop`, or just
delete the matching line.
If you want to fold two or more commits into one, replace the command
-"pick" for the second and subsequent commits with "squash" or "fixup".
+"pick" for the second and subsequent commits with `squash` or `fixup`.
If the commits had different authors, the folded commit will be
attributed to the author of the first commit. The suggested commit
message for the folded commit is the concatenation of the commit
@@ -693,7 +693,7 @@ $ git rebase -i -p --onto Q O
Reordering and editing commits usually creates untested intermediate
steps. You may want to check that your history editing did not break
anything by running a test, or at least recompiling at intermediate
-points in history by using the "exec" command (shortcut "x"). You may
+points in history by using the `exec` command (shortcut `x`). You may
do so by creating a todo list like this one:
---
-- 8< --
--
Duy
Re: [PATCH] rebase -i: introduce the 'test' command
On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote: > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: > > > > > Would it not make more sense to add a command-line option (and a config > > > > setting) to re-schedule failed `exec` commands? Like so: > > > > > > Your proposition would do in most cases, however it is not possible to > > > make a distinction between reschedulable and non-reschedulable commands. > > > > True. But I don't think that's so terrible. > > > > What I think is something to avoid is two commands that do something very, > > very similar, but with two very, very different names. > > > > 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. > > I wonder how often this kind of "yes, I know it fails, but keep going > anyway" situation would come up. And what the interface is like for > getting past it. E.g., what if you fixed a bunch of stuff but your tests > still fail? You may not want to abandon the changes you've made, but you > need to "rebase --continue" to move forward. I encounter this often when > the correct fix is actually in an earlier commit than the one that > yields the test failure. You can't rewind an interactive rebase, so I > complete and restart it, adding an "e"dit at the earlier commit. In this sort of situation, I often whish to be able to do nested rebases. Even more because it happen relatively often that I forget that I'm working in a rebase and not on the head, and then it's quite natural to me to type things like 'git rebase -i @^^^' while already rebasing. But I suppose this has already been discussed. -- Luc
Re: [PATCH] rebase -i: introduce the 'test' command
On 01/12/2018 20:02, Jeff King wrote: On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: Would it not make more sense to add a command-line option (and a config setting) to re-schedule failed `exec` commands? Like so: Your proposition would do in most cases, however it is not possible to make a distinction between reschedulable and non-reschedulable commands. True. But I don't think that's so terrible. What I think is something to avoid is two commands that do something very, very similar, but with two very, very different names. 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. I wonder how often this kind of "yes, I know it fails, but keep going anyway" situation would come up. And what the interface is like for getting past it. E.g., what if you fixed a bunch of stuff but your tests still fail? You may not want to abandon the changes you've made, but you need to "rebase --continue" to move forward. I encounter this often when the correct fix is actually in an earlier commit than the one that yields the test failure. You can't rewind an interactive rebase, so I complete and restart it, adding an "e"dit at the earlier commit. How would I move past the test that fails to continue? I guess "git rebase --edit-todo" and then manually remove it (and any other remaining test lines)? Perhaps we could teach git rebase --skip to skip a rescheduled command, it could be useful if people want to skip rescheduled picks as well (though I don't think I've ever had that happen in the wild). I can see myself turning on the rescheduling config setting but occasionally wanting to be able to skip over the rescheduled exec command. Best Wishes Phillip That's not too bad, but I wonder if people would find it more awkward than the current way (which is to just "rebase --continue" until you get to the end). I dunno. I am not sure if I am for or against changing the default, so these are just some musings. :) -Peff
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 Fri, 30 Nov 2018 at 10:32, Eric Sunshine wrote:
>
> On Thu, Nov 29, 2018 at 11:27 PM Junio C Hamano wrote:
> > Junio C Hamano writes:
> > 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).
> 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.
(If the patch below makes any sense to you and you know more about this
diff/color thing, the fourth bullet in the log message below might
interest you.)
> > 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).
This seems to be an important remark. There's a pretty bad regression
here since when `diffopt` is NULL, we've lost our original, intended
`diffopt->file` and the range-diff ends up on stdout.
Here's my whitespace-damaged WIP. I would be able to pick this up again
in about 6h, but anyone is more than welcome to pick this up and run
with it in the meantime.
This is not a corner of the code that I'm particularly familiar with...
-->8--
Subject: [PATCH] range-diff: always pass at least minimal diff options
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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 create a "dummy" set of diff options where they only fill
in which file to use.
A couple of remarks about this commit:
* No tests. The change in builtin/log.c has been tested manually, the
one in log-tree.c not at all, other than by running existing tests.
* I have not convinced myself 100% that there aren't other things that
are just as important as `file` to pass down.
* `show_range_diff()` can still take NULL, although that is now dead
code. If something like this here commit is deemed the proper fix
for this, that code path could also go, either as part of this
commit, or separately, once we've cut 2.20.
* The range-diff is written colored regardless of destination, i.e.,
when written to a file, it contains garbage.
Signed-off-by: Martin Ågren
---
builtin/log.c | 12 +++-
log-tree.c| 12 +++-
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 5ac18e2848..0609e41ae5 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1094,9 +1094,19 @@ static void make_cover_letter(struct rev_info
*rev, int use_stdout,
}
if (rev->rdiff1) {
+/**
+ * (At least for now) we only want to pass down
+ * the file handle where we want the range-diff
+ * to appear. Avoid any other diff options until
+ * we know how we want to handle them.
+ */
+struct diff_options opts;
+diff_setup();
+opts.file = rev->diffopt.file;
+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..bc355a4e91 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -755,14 +755,24 @@ 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);
+/**
+ * (At least for now) we only want to pass down
+ * the file handle where we want the range-diff
+ * to appear. Avoid any other diff options until
+ * we know how
Re: [RFC] git clean --local
> > Would something like git clean --exclude=file-pattern work as a > > compromise notion? Files matching the pattern would not be cleaned > > regardless of .gitignore or their potential preciousness status > > long-term. Multiple repetitions of the --exclude option might be > > supportable. I could see that being somewhat useful in scripting. > > I think "git clean" already takes "-e", but I am not sure if it is > meant to do what you wrote above. It does already take --exclude=file-pattern, which is like adding lines to .gitignore. (W/o -x/-X, that would mean DON'T clean them, but with -x/-X, it means DO clean them.) > > If "git clean" takes a pathspec, perhaps you can give a negative > pathspec to exclude whatever you do not want to get cleaned, > something like > > git clean '*.o' ':!precious.o' > I like this, but I'm also 100% satisfied with Junio's previous suggestion: git -c core.excludesFile=/dev/null clean ...
Re: [PATCH] t5004: avoid using tar for empty packages
Carlo Marcelo Arenas Belón writes:
> ea2d20d4c2 ("t5004: avoid using tar for checking emptiness of archive",
> 2013-05-09), introduced a fake empty tar archive to allow for portable
> tests of emptiness without having to invoke tar
>
> 4318094047 ("archive: don't add empty directories to archives", 2017-09-13)
> changed the expected result for its tests from one containing an empty
> directory to a plain empty archive but the portable test wasn't updated
> resulting on them failing again in (at least) NetBSD and OpenBSD
>
> Signed-off-by: Carlo Marcelo Arenas Belón
> ---
> t/t5004-archive-corner-cases.sh | 17 -
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
> index ced44355ca..271eb5a1fd 100755
> --- a/t/t5004-archive-corner-cases.sh
> +++ b/t/t5004-archive-corner-cases.sh
> @@ -3,8 +3,12 @@
> test_description='test corner cases of git-archive'
> . ./test-lib.sh
>
> -test_expect_success 'create commit with empty tree' '
> - git commit --allow-empty -m foo
> +# the 10knuls.tar file is used to test for an empty git generated tar
> +# without having to invoke tar because an otherwise valid empty GNU tar
> +# will be considered broken by {Open,Net}BSD tar
> +test_expect_success 'create commit with empty tree and fake empty tar' '
> + git commit --allow-empty -m foo &&
> + perl -e "print \"\\0\" x 10240" >10knuls.tar
> '
OK, so you moved the creation of the file with block of NULs to the
set-up phase of the entire script.
> # Make a dir and clean it up afterwards
> @@ -47,7 +51,6 @@ test_expect_success HEADER_ONLY_TAR_OK 'tar archive of
> commit with empty tree' '
>
> test_expect_success 'tar archive of empty tree is empty' '
> git archive --format=tar HEAD: >empty.tar &&
> - perl -e "print \"\\0\" x 10240" >10knuls.tar &&
> test_cmp_bin 10knuls.tar empty.tar
> '
And because of that, this one that was added for ea2d20d4 ("t5004:
avoid using tar for checking emptiness of archive", 2013-05-09) is
now simplified. It can use the one that was created in the set-up
phase.
> @@ -106,16 +109,12 @@ test_expect_success 'create a commit with an empty
> subtree' '
>
> test_expect_success 'archive empty subtree with no pathspec' '
> git archive --format=tar $root_tree >subtree-all.tar &&
> - make_dir extract &&
> - "$TAR" xf subtree-all.tar -C extract &&
> - check_dir extract
> + test_cmp_bin 10knuls.tar subtree-all.tar
> '
And then we avoid the test that assumes that an empty tar archive
can safely and portably extracted, and instead check the emptiness
the same way as the earlier test here ...
> test_expect_success 'archive empty subtree by direct pathspec' '
> git archive --format=tar $root_tree -- sub >subtree-path.tar &&
> - make_dir extract &&
> - "$TAR" xf subtree-path.tar -C extract &&
> - check_dir extract
> + test_cmp_bin 10knuls.tar subtree-path.tar
> '
... and here, too.
OK, and the result is consistent with the "We can help GNU and BSD
tar, but NetBSD tar cannot be salvageable" approach, laid out in the
earlier ea2d20d4 ("t5004: avoid using tar for checking emptiness of
archive", 2013-05-09). Makes sense.
Thanks.
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Frank Schäfer writes: > Hi Junio, > > Am 29.11.18 um 03:11 schrieb Junio C Hamano: > [...] >> This was misspoken a bit. Let's revise it to >> >> When producing a colored output (not limited to whitespace >> error coloring of diff output) for contents that are not >> marked as eol=crlf (and other historical means), insert >> before a CR that comes immediately before a LF. > You mean > ... > *after* a CR that comes immediately before a LF." > > OK, AFAICS this produces the desired output in all cases if eol=lf. OK, yeah, I think I meant "after", i.e. ... CR LF, in order to force CR to be separated from LF. > Now what about the case eol=crlf ? I have no strong opinions, other than that "LF in repository, CRLF in working tree" would make the issue go away (when it is solved for EOL=LF like the above, that is). > Keeping the current behavior of '-' lines is correct. > But shouldn't ^M now be suppressed in '+' lines for a consistent behavior ? If "LF in repository, CRLF in working tree" is done, there would not be ^M at the end of the line, not just for removed lines, but also for added lines, no?
Re: [PATCH] Do not fail test if '.' is part of $PATH
"H.Merijn Brand" writes: > When $PATH contains the current directory as .:PATH, PATH:., PATH:.:PATH, > or (maybe worse) as :PATH, PATH:, or PATH::PATH - as an empty entry is > identical to having dot in $PATH - this test used to fail It is totally unclear what "this test" refers to. Let's retitle it to > Subject: [PATCH] t0061: do not fail test if '.' is part of $PATH and do something like this: t0061 created a script named with an unlikely name in the current directory to ensure that it is not found via the run_command() API, expecting that $PATH does not contain an element that names the current directory (i.e. '.' or '') in a sane environment. This obviously would not work if the $PATH does contain such an element. Introduce a DOT_IN_PATH lazy prerequisite to catch such a case and skip the test when the environment is not so sane. > +test_lazy_prereq DOT_IN_PATH ' > + case ":$PATH:" in > + *:.:*|*::*) true ;; > + *) false ;; > + esac > +' > + > +test_expect_success !DOT_IN_PATH 'run_command is restricted to PATH' ' > write_script should-not-run <<-\EOF && > echo yikes > EOF I also like Peff's more straight-forward approach that avoids looking into PATH but instead ask the shell what we care about (i.e. would we end up running 'should-not-run' if we asked the system to run it without giving an explicit path to it?). The last paragraph of the above would need to change if we were to go in that direction to something like Check if the running shell picks up the script without an explicit path to it and skip the test when it does. perhaps. The code to do so got a bit more compact than what Peff wrote but I think it still retains its main beauty, which is how straight-forward it is. t/t0061-run-command.sh | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh index cf932c8514..17b560370e 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -29,7 +29,15 @@ test_expect_success 'run_command can run a command' ' test_must_be_empty err ' -test_expect_success 'run_command is restricted to PATH' ' + +test_lazy_prereq RUNS_COMMANDS_FROM_PWD ' + write_script runs-commands-from-pwd <<-\EOF && + true + EOF + runs-commands-from-pwd >/dev/null 2>&1 +' + +test_expect_success !RUNS_COMMANDS_FROM_PWD 'run_command is restricted to PATH' ' write_script should-not-run <<-\EOF && echo yikes EOF
Re: [PATCH] Do not fail test if '.' is part of $PATH
Jeff King writes: > Since the test is ultimately checking "can we run should-not-run from > the current directory", might it be simpler to actually try that as the > precondition? I.e., something like: > ... A nice egg of columbus. It also would save us from mischievous users who have should-not-run somewhere no the $PATH that outputs the string we expect (no, I do not think it is a common thing to do; I am just saying that the solution covers such an extremely stupid case without special casing).
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 02.12.18 um 20:31 schrieb Frank Schäfer: Am 29.11.18 um 03:11 schrieb Junio C Hamano: [...] This was misspoken a bit. Let's revise it to When producing a colored output (not limited to whitespace error coloring of diff output) for contents that are not marked as eol=crlf (and other historical means), insert before a CR that comes immediately before a LF. You mean ... *after* a CR that comes immediately before a LF." OK, AFAICS this produces the desired output in all cases if eol=lf. Now what about the case eol=crlf ? Keeping the current behavior of '-' lines is correct. But shouldn't ^M now be suppressed in '+' lines for a consistent behavior ? That can be achieved with whitespace=cr-at-eol. With other words: "If CR comes immediately before a LF, do the following with *all* lines: after the CR if eol=lf but do not after the CR if eol=crlf." Why? It is the pager's duty to highlight CR, IMO. If it doesn't, but the user wants to see them, then they are using the wrong pager or the wrong pager settings. As far as I am concerned, I do not have any of my files marked as eol=crlf, but I do *not* want to see ^M in the pager. I.e., having git insert between CR and LF would do the wrong thing for me. -- Hannes
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Peff, On Sat, 1 Dec 2018, Jeff King wrote: > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: > > > > > Would it not make more sense to add a command-line option (and a config > > > > setting) to re-schedule failed `exec` commands? Like so: > > > > > > Your proposition would do in most cases, however it is not possible to > > > make a distinction between reschedulable and non-reschedulable commands. > > > > True. But I don't think that's so terrible. > > > > What I think is something to avoid is two commands that do something very, > > very similar, but with two very, very different names. > > > > 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. Yep, `break`, as Eric pointed out. After all, you did not really want a command to fail, you just wanted the interactive rebase to give you a break. > I wonder how often this kind of "yes, I know it fails, but keep going > anyway" situation would come up. And what the interface is like for > getting past it. E.g., what if you fixed a bunch of stuff but your tests > still fail? You may not want to abandon the changes you've made, but you > need to "rebase --continue" to move forward. I encounter this often when > the correct fix is actually in an earlier commit than the one that > yields the test failure. You can't rewind an interactive rebase, so I > complete and restart it, adding an "e"dit at the earlier commit. > > How would I move past the test that fails to continue? I guess "git > rebase --edit-todo" and then manually remove it (and any other remaining > test lines)? Yes, the current way would be to use `git rebase --edit-todo`. > That's not too bad, but I wonder if people would find it more awkward > than the current way (which is to just "rebase --continue" until you get > to the end). > > I dunno. I am not sure if I am for or against changing the default, so > these are just some musings. :) It's good that you chimed in with your side of things. If you missed the `break` command, so will many others have missed it. And continue to miss it. Besides, Junio mentioned elsewhere that he is in the camp of people who wait for enough users to complain why some config option isn't the default to actually change the default. So... I guess we'll leave the default where it is for now. Ciao, Dscho
Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files
Thomas Gummerer writes:
> Agreed, I think --{no-,}overlay is a much better name for the option,
> I'll use that for my patch series (I hope to send that soon after 2.20
> is released).
OK.
> I must admit that I was not aware that the mode is called overlay
> mode, before you explained it to me, so I wouldn't expect most users
> to know either. But as it's easy to explain that probably doesn't
> matter much.
I do not think "the mode is called the overlay mode" is so accurate
a description. I think I've seen the word 'overlay' used to
describe the behaviour in earlier discussions, but because there is
no 'non-overlay' mode exists in versions of 'git checkout' the
end-users have, the users won't even be aware of the possibility
that mode different from what they are used to see could exist, or
that the mode that they are used to see could be called/explained as
the 'overlay' mode. IOW, we should pick the best phrase to explain
the behaviour we can use when coming up with the command line
option, and that phrase does not have to be 'overlay'---there is no
"using the word 'overlay' for this is good because the users are
familiar with the existing use of the word", simply because there
isn't such familiarilty ;-)
Re: [L10N] Kickoff for Git 2.20.0 round 3
Jiang Xin writes: > Git v2.20.0-rc2 has been released, and there are 5 new messages need to > be translated. So let's start new round of l10n for Git 2.20.0. A huge thanks, as always, to the translation team. Jiang, sorry to see that -rc2 slipped just after you sent out the round 2 message and you needed to start round 3 just after it.
Re: [RFC] git clean --local
"Randall S. Becker" writes: > Would something like git clean --exclude=file-pattern work as a > compromise notion? Files matching the pattern would not be cleaned > regardless of .gitignore or their potential preciousness status > long-term. Multiple repetitions of the --exclude option might be > supportable. I could see that being somewhat useful in scripting. I think "git clean" already takes "-e", but I am not sure if it is meant to do what you wrote above. If "git clean" takes a pathspec, perhaps you can give a negative pathspec to exclude whatever you do not want to get cleaned, something like git clean '*.o' ':!precious.o' to say "presious.o is ignored (hence normally expendable), but I do not want to clean it with this invocation of 'git clean'"?
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Hi Junio, Am 29.11.18 um 03:11 schrieb Junio C Hamano: [...] > This was misspoken a bit. Let's revise it to > > When producing a colored output (not limited to whitespace > error coloring of diff output) for contents that are not > marked as eol=crlf (and other historical means), insert >before a CR that comes immediately before a LF. You mean ... *after* a CR that comes immediately before a LF." OK, AFAICS this produces the desired output in all cases if eol=lf. Now what about the case eol=crlf ? Keeping the current behavior of '-' lines is correct. But shouldn't ^M now be suppressed in '+' lines for a consistent behavior ? With other words: "If CR comes immediately before a LF, do the following with *all* lines: after the CR if eol=lf but do not after the CR if eol=crlf." Regards, Frank
Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files
On 11/30, Junio C Hamano wrote:
>
> I am unsure about the wisdom of calling it "--index", though.
>
> The "--index" option is "the command can work only on the index, or
> only on the working tree files, or on both the index and the working
> tree files, and this option tells it to work in the 'both the index
> and the working tree files' mode", but when "restore-files" touches
> paths, it always modifies both the index and the working tree, so
> the "--index" option does not capture the differences well in this
> context [*1*]. As I saw this was described as "not using the usual
> 'overlay' semantics [*2*]", perhaps --overlay/--no-overlay option
> that defaults to --no-overlay is easier to explain.
Agreed, I think --{no-,}overlay is a much better name for the option,
I'll use that for my patch series (I hope to send that soon after 2.20
is released).
I must admit that I was not aware that the mode is called overlay
mode, before you explained it to me, so I wouldn't expect most users
to know either. But as it's easy to explain that probably doesn't
matter much.
> side note 1. I think the original mention of "--index" came in
> the context of contrasting "git reset" with "git checkout".
> "git reset (--hard|--mixed) -- " (that does not move
> HEAD), which does not but perhaps should exist, is very much
> like "git checkout -- ", and if "reset" were written
> after the "--index/--cached" convention was established, "reset
> --hard" would have called "reset --index" while "reset --mixed"
> would have been "reset --cached" (i.e. only work on the index
> and not on the working tree). And "reset --index "
> would have worked by removing paths in that are not
> in the HEAD and updating paths in that are in the
> HEAD, i.e. identical to the non overlay behaviour proposed for
> the "git checkout" command. So calling the non overlay mode
> "--index" makes sense in the context of discussing "git reset",
> and not in the context of "git checkout".
>
> side note 2. "git checkout " grabs entries
> from the that patch and adds them to the
> index and checks them out to the working tree. If the original
> index has entries that match but do not appear in
> , they are left in the result. That is "overlaying
> what was taken from the on top of what is in the
> index".
>
> Having said all that, I will not be looking at the series until 2.20
> final. And I hope more people do the same to concentrate on helping
> us prevent the last minute glitch slipping in the final release.
>
> Thanks.
RE: [RFC] git clean --local
On December 2, 2018 8:26, Ævar Arnfjörð Bjarmason wrote: > > On Sat, Dec 01 2018, Cameron Boehmer wrote: > > > 1) add a new flag > > -l, --local > > Do not consult git config --global core.excludesFile in > > determining what files git ignores. This is useful in conjunction with > > -x/-X to preserve user files while removing build artifacts. > > Or perhaps a general flag to ignore configuration would be useful for such > cases, see https://public- > inbox.org/git/87zhtqvm66@evledraar.gmail.com/ Would something like git clean --exclude=file-pattern work as a compromise notion? Files matching the pattern would not be cleaned regardless of .gitignore or their potential preciousness status long-term. Multiple repetitions of the --exclude option might be supportable. I could see that being somewhat useful in scripting. Cheers, Randall
Re: "git add -p" versus "git add -i", followed by "p"
On Sun, 2 Dec 2018, Duy Nguyen wrote: > On Sun, Dec 2, 2018 at 6:05 PM Robert P. J. Day wrote: > > > Patch update>> 2 > > > staged unstaged path > > > * 1:unchanged+1/-0 README.md > > > * 2:unchanged+1/-0 contrib/README > > > 3:unchanged+1/-0 t/README > > > Patch update>> > > > > > > Here I hit enter. Did you? > > > > perhaps i'm just not seeing it, but from "man git-add", it > > doesn't seem obvious that you would first select the files to work > > with, then hit a simple CR to get into actual patch mode. > > I think it's the same procedure as the "update" step, which > describes this in more detail. I agree that the "patch" section does > not make this obvious. thanks, i was hoping i wasn't being a complete idiot. given time, i may submit a patch to fix the man page unless someone else gets to it first. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git add -p" versus "git add -i", followed by "p"
On Sun, Dec 2, 2018 at 6:05 PM Robert P. J. Day wrote: > > Patch update>> 2 > > staged unstaged path > > * 1:unchanged+1/-0 README.md > > * 2:unchanged+1/-0 contrib/README > > 3:unchanged+1/-0 t/README > > Patch update>> > > > > Here I hit enter. Did you? > > perhaps i'm just not seeing it, but from "man git-add", it doesn't > seem obvious that you would first select the files to work with, then > hit a simple CR to get into actual patch mode. I think it's the same procedure as the "update" step, which describes this in more detail. I agree that the "patch" section does not make this obvious. -- Duy
Re: "git add -p" versus "git add -i", followed by "p"
On Sun, 2 Dec 2018, SZEDER Gábor wrote: > On Sun, Dec 02, 2018 at 11:30:19AM -0500, Robert P. J. Day wrote: > > > > testing adding by patch for the very first time (i've just never > > needed this), and reading the "progit" book and reading the man page, > > and the impression i'm getting is that running "git add -p" (going > > straight to patch mode) is supposed to be equivalent to running "git > > add -i", then typing "p" to switch to patch mode. > > > > that is most emphatically not what i'm seeing. if i run "git add > > -p", then i get to what i expect -- the patch subsystem: > > > > $ git add -p > > diff --git a/README.asc b/README.asc > > index fa40bad..840e85b 100644 > > --- a/README.asc > > +++ b/README.asc > > @@ -1,3 +1,9 @@ > > +change 1 > > + > > + > > + > > + > > + > >= Pro Git, Second Edition > > > >Welcome to the second edition of the Pro Git book. > > Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? > > > > but if i start with "git add -i", there seems to be no way to get to > > patch mode -- certainly "p" doesn't do it. am i stupidly missing > > something trivial? is the explanation misleading or inncomplete? > > Worksforme™: > > $ echo "New content" >>README.md > $ echo "New content" >>t/README > $ echo "New content" >>contrib//README > $ git add -i > staged unstaged path > 1:unchanged+1/-0 README.md > 2:unchanged+1/-0 contrib/README > 3:unchanged+1/-0 t/README > > *** Commands *** > 1: status 2: update 3: revert 4: add untracked > 5: patch6: diff 7: quit 8: help > What now> p > staged unstaged path > 1:unchanged+1/-0 README.md > 2:unchanged+1/-0 contrib/README > 3:unchanged+1/-0 t/README > Patch update>> 1 > staged unstaged path > * 1:unchanged+1/-0 README.md > 2:unchanged+1/-0 contrib/README > 3:unchanged+1/-0 t/README > Patch update>> 2 > staged unstaged path > * 1:unchanged+1/-0 README.md > * 2:unchanged+1/-0 contrib/README > 3:unchanged+1/-0 t/README > Patch update>> > > Here I hit enter. Did you? perhaps i'm just not seeing it, but from "man git-add", it doesn't seem obvious that you would first select the files to work with, then hit a simple CR to get into actual patch mode. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git add -p" versus "git add -i", followed by "p"
On Sun, 2 Dec 2018, SZEDER Gábor wrote: > On Sun, Dec 02, 2018 at 11:30:19AM -0500, Robert P. J. Day wrote: > > > > testing adding by patch for the very first time (i've just never > > needed this), and reading the "progit" book and reading the man page, > > and the impression i'm getting is that running "git add -p" (going > > straight to patch mode) is supposed to be equivalent to running "git > > add -i", then typing "p" to switch to patch mode. > > > > that is most emphatically not what i'm seeing. if i run "git add > > -p", then i get to what i expect -- the patch subsystem: > > > > $ git add -p > > diff --git a/README.asc b/README.asc > > index fa40bad..840e85b 100644 > > --- a/README.asc > > +++ b/README.asc > > @@ -1,3 +1,9 @@ > > +change 1 > > + > > + > > + > > + > > + > >= Pro Git, Second Edition > > > >Welcome to the second edition of the Pro Git book. > > Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? > > > > but if i start with "git add -i", there seems to be no way to get to > > patch mode -- certainly "p" doesn't do it. am i stupidly missing > > something trivial? is the explanation misleading or inncomplete? > > Worksforme™: > > $ echo "New content" >>README.md > $ echo "New content" >>t/README > $ echo "New content" >>contrib//README > $ git add -i > staged unstaged path > 1:unchanged+1/-0 README.md > 2:unchanged+1/-0 contrib/README > 3:unchanged+1/-0 t/README > > *** Commands *** > 1: status 2: update 3: revert 4: add untracked > 5: patch6: diff 7: quit 8: help > What now> p > staged unstaged path > 1:unchanged+1/-0 README.md > 2:unchanged+1/-0 contrib/README > 3:unchanged+1/-0 t/README > Patch update>> 1 > staged unstaged path > * 1:unchanged+1/-0 README.md > 2:unchanged+1/-0 contrib/README > 3:unchanged+1/-0 t/README > Patch update>> 2 > staged unstaged path > * 1:unchanged+1/-0 README.md > * 2:unchanged+1/-0 contrib/README > 3:unchanged+1/-0 t/README > Patch update>> > > Here I hit enter. Did you? ah, so after selecting the files to selectively stage, one enters a simple CR. got it. not sure that's obvious from the docs but i'll verify that. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git add -p" versus "git add -i", followed by "p"
On Sun, 2 Dec 2018, Kevin Daudt wrote: > On Sun, Dec 02, 2018 at 11:30:19AM -0500, Robert P. J. Day wrote: > > > > testing adding by patch for the very first time (i've just never > > needed this), and reading the "progit" book and reading the man page, > > and the impression i'm getting is that running "git add -p" (going > > straight to patch mode) is supposed to be equivalent to running "git > > add -i", then typing "p" to switch to patch mode. > > > > that is most emphatically not what i'm seeing. if i run "git add > > -p", then i get to what i expect -- the patch subsystem: > > > > $ git add -p > > diff --git a/README.asc b/README.asc > > index fa40bad..840e85b 100644 > > --- a/README.asc > > +++ b/README.asc > > @@ -1,3 +1,9 @@ > > +change 1 > > + > > + > > + > > + > > + > >= Pro Git, Second Edition > > > >Welcome to the second edition of the Pro Git book. > > Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? > > > > but if i start with "git add -i", there seems to be no way to get > > to patch mode -- certainly "p" doesn't do it. am i stupidly > > missing something trivial? is the explanation misleading or > > inncomplete? > > After selecting 'p', what do you get? > > You should see a list of modified files. You can select the files > you want to stage by the listed numbers. After you selected those > files, you press enter, and then you will get the options you'll > also see with git add -p. $ git add -i staged unstaged path 1:unchanged +12/-0 README.asc *** Commands *** 1: [s]tatus 2: [u]pdate 3: [r]evert 4: [a]dd untracked 5: [p]atch 6: [d]iff 7: [q]uit 8: [h]elp What now> p staged unstaged path 1:unchanged +12/-0 [R]EADME.asc Patch update>> 1 staged unstaged path * 1:unchanged +12/-0 [R]EADME.asc Patch update>> 1 staged unstaged path * 1:unchanged +12/-0 [R]EADME.asc Patch update>> and ... then what? rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: "git add -p" versus "git add -i", followed by "p"
On Sun, Dec 02, 2018 at 11:30:19AM -0500, Robert P. J. Day wrote: > > testing adding by patch for the very first time (i've just never > needed this), and reading the "progit" book and reading the man page, > and the impression i'm getting is that running "git add -p" (going > straight to patch mode) is supposed to be equivalent to running "git > add -i", then typing "p" to switch to patch mode. > > that is most emphatically not what i'm seeing. if i run "git add > -p", then i get to what i expect -- the patch subsystem: > > $ git add -p > diff --git a/README.asc b/README.asc > index fa40bad..840e85b 100644 > --- a/README.asc > +++ b/README.asc > @@ -1,3 +1,9 @@ > +change 1 > + > + > + > + > + >= Pro Git, Second Edition > >Welcome to the second edition of the Pro Git book. > Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? > > but if i start with "git add -i", there seems to be no way to get to > patch mode -- certainly "p" doesn't do it. am i stupidly missing > something trivial? is the explanation misleading or inncomplete? > > rday > > -- > After selecting 'p', what do you get? You should see a list of modified files. You can select the files you want to stage by the listed numbers. After you selected those files, you press enter, and then you will get the options you'll also see with git add -p. Kevin
Re: "git add -p" versus "git add -i", followed by "p"
On Sun, Dec 02, 2018 at 11:30:19AM -0500, Robert P. J. Day wrote: > > testing adding by patch for the very first time (i've just never > needed this), and reading the "progit" book and reading the man page, > and the impression i'm getting is that running "git add -p" (going > straight to patch mode) is supposed to be equivalent to running "git > add -i", then typing "p" to switch to patch mode. > > that is most emphatically not what i'm seeing. if i run "git add > -p", then i get to what i expect -- the patch subsystem: > > $ git add -p > diff --git a/README.asc b/README.asc > index fa40bad..840e85b 100644 > --- a/README.asc > +++ b/README.asc > @@ -1,3 +1,9 @@ > +change 1 > + > + > + > + > + >= Pro Git, Second Edition > >Welcome to the second edition of the Pro Git book. > Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? > > but if i start with "git add -i", there seems to be no way to get to > patch mode -- certainly "p" doesn't do it. am i stupidly missing > something trivial? is the explanation misleading or inncomplete? Worksforme™: $ echo "New content" >>README.md $ echo "New content" >>t/README $ echo "New content" >>contrib//README $ git add -i staged unstaged path 1:unchanged+1/-0 README.md 2:unchanged+1/-0 contrib/README 3:unchanged+1/-0 t/README *** Commands *** 1: status 2: update 3: revert 4: add untracked 5: patch6: diff 7: quit 8: help What now> p staged unstaged path 1:unchanged+1/-0 README.md 2:unchanged+1/-0 contrib/README 3:unchanged+1/-0 t/README Patch update>> 1 staged unstaged path * 1:unchanged+1/-0 README.md 2:unchanged+1/-0 contrib/README 3:unchanged+1/-0 t/README Patch update>> 2 staged unstaged path * 1:unchanged+1/-0 README.md * 2:unchanged+1/-0 contrib/README 3:unchanged+1/-0 t/README Patch update>> Here I hit enter. Did you? diff --git a/README.md b/README.md index f920a42fad..63dee5cfc3 100644 --- a/README.md +++ b/README.md @@ -62,3 +62,4 @@ and the name as (depending on your mood): [Documentation/giteveryday.txt]: Documentation/giteveryday.txt [Documentation/gitcvs-migration.txt]: Documentation/gitcvs-migration.txt [Documentation/SubmittingPatches]: Documentation/SubmittingPatches +New content Stage this hunk [y,n,q,a,d,e,?]? y diff --git a/contrib/README b/contrib/README index 05f291c1f1..2b152dfcff 100644 --- a/contrib/README +++ b/contrib/README @@ -41,3 +41,4 @@ submit a patch to create a subdirectory of contrib/ and put your stuff there. -jc +New content Stage this hunk [y,n,q,a,d,e,?]? n *** Commands *** 1: status 2: update 3: revert 4: add untracked 5: patch6: diff 7: quit 8: help What now> q Bye. $ git diff --cached diff --git a/README.md b/README.md index f920a42fad..63dee5cfc3 100644 --- a/README.md +++ b/README.md @@ -62,3 +62,4 @@ and the name as (depending on your mood): [Documentation/giteveryday.txt]: Documentation/giteveryday.txt [Documentation/gitcvs-migration.txt]: Documentation/gitcvs-migration.txt [Documentation/SubmittingPatches]: Documentation/SubmittingPatches +New content $ Arguably the documentation could make it clear that the user can choose multiple files at once, e.g.: diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index c9623854bf..061f9cbb0d 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -317,9 +317,9 @@ add untracked:: patch:: - This lets you choose one path out of a 'status' like selection. - After choosing the path, it presents the diff between the index - and the working tree file and asks you if you want to stage + This lets you choose one or more paths out of a 'status' like selection. + After choosing the path(s), it presents the diff between the index + and the working tree file(s) and asks you if you want to stage the change of each hunk. You can select one of the following options and type return: And perhaps we could have a dedicated menu entry for "I'm done with selecting paths"? Dunno; I'm a 'git add -p' user myself.
Re: easy way to demonstrate length of colliding SHA-1 prefixes?
On Sun, 2 Dec 2018, Ævar Arnfjörð Bjarmason wrote:
> On Sun, Dec 02 2018, Robert P. J. Day wrote:
>
> > as part of an upcoming git class i'm delivering, i thought it
> > would be amusing to demonstrate the maximum length of colliding
> > SHA-1 prefixes in a repository (in my case, i use the linux kernel
> > git repo for most of my examples).
> >
> > is there a way to display the objects in the object database
> > that clash in the longest object name SHA-1 prefix; i mean, short
> > of manually listing all object names, running that through cut and
> > sort and uniq and ... you get the idea.
> >
> > is there a cute way to do that? thanks.
>
> You'll always need to list them all. It's inherently an operation
> where for each SHA-1 you need to search for other ones with that
> prefix up to a given length.
i assumed as much, just wasn't sure about the esoteric dark corners
of git i've never gotten to yet.
> Perhaps you've missed that you can use --abbrev=N for this, and just
> grep for things that are loger than that N, e.g. for linux.git:
>
> git log --oneline --abbrev=10 --pretty=format:%h |
> grep -E -v '^.{10}$' |
> perl -pe 's/^(.{10}).*/$1/'
>
> This will list the 4 objects that need more than 10 characters to be
> shown unambiguously. If you then "git cat-file -t" them you'll get
> the disambiguation help.
that's pretty close to what i came up with, thanks.
rday
--
Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca/dokuwiki
Twitter: http://twitter.com/rpjday
LinkedIn: http://ca.linkedin.com/in/rpjday
Re: [PATCH v3 01/42] parse-options: support --git-completion-helper
On Fri, Feb 09 2018, Nguyễn Thái Ngọc Duy wrote:
> +static int show_gitcomp(struct parse_opt_ctx_t *ctx,
> + const struct option *opts)
> +{
Says it returns 'static int'...
> [...]
> + exit(0);
Then just exits...
> + /* lone --git-completion-helper is asked by git-completion.bash
> */
> + if (ctx->total == 1 && !strcmp(arg + 1,
> "-git-completion-helper"))
> + return show_gitcomp(ctx, options);
This return value is never used.
Whine from SunCC (not fatal, also this was in v2.17.0 so no need to fix
before 2.20, just saw this now):
"parse-options.c", line 520: warning: Function has no return
statement : show_gitcomp
Re: [RFC] git clean --local
On Sat, Dec 01 2018, Cameron Boehmer wrote: > 1) add a new flag > -l, --local > Do not consult git config --global core.excludesFile in > determining what files git ignores. This is useful in conjunction with > -x/-X to preserve user files while removing build artifacts. Or perhaps a general flag to ignore configuration would be useful for such cases, see https://public-inbox.org/git/87zhtqvm66@evledraar.gmail.com/
Re: easy way to demonstrate length of colliding SHA-1 prefixes?
On Sun, Dec 02 2018, Robert P. J. Day wrote:
> as part of an upcoming git class i'm delivering, i thought it would
> be amusing to demonstrate the maximum length of colliding SHA-1
> prefixes in a repository (in my case, i use the linux kernel git repo
> for most of my examples).
>
> is there a way to display the objects in the object database that
> clash in the longest object name SHA-1 prefix; i mean, short of
> manually listing all object names, running that through cut and sort
> and uniq and ... you get the idea.
>
> is there a cute way to do that? thanks.
You'll always need to list them all. It's inherently an operation where
for each SHA-1 you need to search for other ones with that prefix up to
a given length.
Perhaps you've missed that you can use --abbrev=N for this, and just
grep for things that are loger than that N, e.g. for linux.git:
git log --oneline --abbrev=10 --pretty=format:%h |
grep -E -v '^.{10}$' |
perl -pe 's/^(.{10}).*/$1/'
This will list the 4 objects that need more than 10 characters to be
shown unambiguously. If you then "git cat-file -t" them you'll get the
disambiguation help.
Re: [PATCH 8/9] sha1-file: use loose object cache for quick existence check
Am 13.11.2018 um 11:02 schrieb Ævar Arnfjörð Bjarmason: > > On Mon, Nov 12 2018, Ævar Arnfjörð Bjarmason wrote: > >> On Mon, Nov 12 2018, Ævar Arnfjörð Bjarmason wrote: >> >>> I get: >>> >>> Test origin/master >>> peff/jk/loose-cache avar/check-collisions-config >>> >>> >>> 0008.2: index-pack with 256*1 loose objects 4.31(0.55+0.18) >>> 0.41(0.40+0.02) -90.5% 0.23(0.36+0.01) -94.7% >>> 0008.3: index-pack with 256*10 loose objects 4.37(0.45+0.21) >>> 0.45(0.40+0.02) -89.7% 0.25(0.38+0.01) -94.3% >>> 0008.4: index-pack with 256*100 loose objects4.47(0.53+0.23) >>> 0.67(0.63+0.02) -85.0% 0.24(0.38+0.01) -94.6% >>> 0008.5: index-pack with 256*250 loose objects5.01(0.67+0.30) >>> 1.04(0.98+0.06) -79.2% 0.24(0.37+0.01) -95.2% >>> 0008.6: index-pack with 256*500 loose objects5.11(0.57+0.21) >>> 1.81(1.70+0.09) -64.6% 0.25(0.38+0.01) -95.1% >>> 0008.7: index-pack with 256*750 loose objects5.12(0.60+0.22) >>> 2.54(2.38+0.14) -50.4% 0.24(0.38+0.01) -95.3% >>> 0008.8: index-pack with 256*1000 loose objects 4.52(0.52+0.21) >>> 3.36(3.17+0.17) -25.7% 0.23(0.36+0.01) -94.9% >>> >>> I then hacked it to test against git.git, but skipped origin/master for >>> that one because it takes *ages*. So just mine v.s. yours: >>> >>> $ GIT_PERF_REPEAT_COUNT=3 GIT_PERF_MAKE_OPTS='-j56 CFLAGS="-O3"' ./run >>> peff/jk/loose-cache avar/check-collisions-config p0008-index-pack.sh >>> [...] >>> Test peff/jk/loose-cache >>> avar/check-collisions-config >>> >>> --- >>> 0008.2: index-pack with 256*1 loose objects 12.57(28.72+0.61) >>> 12.68(29.36+0.62) +0.9% >>> 0008.3: index-pack with 256*10 loose objects 12.77(28.75+0.61) >>> 12.50(28.88+0.56) -2.1% >>> 0008.4: index-pack with 256*100 loose objects13.20(29.49+0.66) >>> 12.38(28.58+0.60) -6.2% >>> 0008.5: index-pack with 256*250 loose objects14.10(30.59+0.64) >>> 12.54(28.22+0.57) -11.1% >>> 0008.6: index-pack with 256*500 loose objects14.48(31.06+0.74) >>> 12.43(28.59+0.60) -14.2% >>> 0008.7: index-pack with 256*750 loose objects15.31(31.91+0.74) >>> 12.67(29.23+0.64) -17.2% >>> 0008.8: index-pack with 256*1000 loose objects 16.34(32.84+0.76) >>> 13.11(30.19+0.68) -19.8% >>> >>> So not much of a practical difference perhaps. But then again this isn't >>> a very realistic test case of anything. Rarely are you going to push a >>> history of something the size of git.git into a repo with this many >>> loose objects. >>> >>> Using sha1collisiondetection.git is I think the most realistic scenario, >>> i.e. you'll often end up fetching/pushing something roughly the size of >>> its entire history on a big repo, and with it: >>> >>> Test peff/jk/loose-cache >>> avar/check-collisions-config >>> >>> --- >>> 0008.2: index-pack with 256*1 loose objects 0.16(0.04+0.01) >>> 0.05(0.03+0.00) -68.8% >>> 0008.3: index-pack with 256*10 loose objects 0.19(0.04+0.02) >>> 0.05(0.02+0.00) -73.7% >>> 0008.4: index-pack with 256*100 loose objects0.32(0.17+0.02) >>> 0.04(0.02+0.00) -87.5% >>> 0008.5: index-pack with 256*250 loose objects0.57(0.41+0.03) >>> 0.04(0.02+0.00) -93.0% >>> 0008.6: index-pack with 256*500 loose objects1.02(0.83+0.06) >>> 0.04(0.03+0.00) -96.1% >>> 0008.7: index-pack with 256*750 loose objects1.47(1.24+0.10) >>> 0.04(0.02+0.00) -97.3% >>> 0008.8: index-pack with 256*1000 loose objects 1.94(1.70+0.10) >>> 0.04(0.02+0.00) -97.9% >>> >>> As noted in previous threads I have an in-house monorepo where (due to >>> expiry policies) loose objects hover around the 256*250 mark
Re: Confusing inconsistent option syntax
On Sun, Dec 2, 2018 at 11:13 AM Robert White wrote: > > `git log --pretty short` gives the error message "ambiguous argument > 'short'". To get the expected result, you need to use `git log > --pretty=short`. However, `git log --since yesterday` and `git log > --since=yesterday` both work as expected. > > When is an = needed? What is the reason for these inconsistencies? --pretty can take no arguments. --pretty alone is the same as --pretty=medium. --since always needs an argument. -- Duy
Re: [RFC] git clean --local
Junio C Hamano writes: > Cameron Boehmer writes: > >> 1) add a new flag >> -l, --local >> Do not consult git config --global core.excludesFile in >> determining what files git ignores. This is useful in conjunction with >> -x/-X to preserve user files while removing build artifacts. > ... > But it might be useful as an option that affects any "git" command, > e.g. "git --local-config-only clean". I dunno. If you only want to say "there is no global excludes file", perhaps $ git -c core.excludesFile=/dev/null clean -x may be sufficient, so for that particular use case, there is no need to introduce a new command, I'd think. In the longer term, however, I think we would want to introduce a distinction among ignored files---we only support "ignored and expendable" class, but not "ignored but precious" class. With the latter class introduced, it would make sense for "git clean -x/-X" to notice that a path is ignored but precious and keep it. If a dir/foo is ignored, dir/bar is tracked in commit A that is currently checked out, and there is no dir/ directory in commit B, checking out commit B would remove dir/foo (because the last tracked file in the directory goes away and all remaining files in the directory would be ignored but expendable). But if we introduced a new "ignored but precious" class and made dir/foo a member of such a class, then you will be prevented from checkout out B until you do something about dir/foo that is now "precious".
Re: [PATCH] rebase -i: introduce the 'test' command
On Sat, Dec 01, 2018 at 09:28:47PM -0500, Eric Sunshine wrote: > 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) Yes, thanks (as you can see, I haven't actually used it yet ;) ). -Peff
Re: [PATCH 1/2] config.mak.uname: OpenBSD uses BSD semantics with fread for directories
FWIW this patch doesn't have any other siblings and subject should had been just [PATCH]; apologize for the confusion and the spam (including that other duplicated email, and most likely this one) Carlo
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: [RFC] git clean --local
Cameron Boehmer writes: > 1) add a new flag > -l, --local > Do not consult git config --global core.excludesFile in > determining what files git ignores. This is useful in conjunction with > -x/-X to preserve user files while removing build artifacts. This does not belong to the "clean" command (who says the need to ignore the global configuration is limited to "clean" and why?), so "git clean --local" is simply not acceptable. But it might be useful as an option that affects any "git" command, e.g. "git --local-config-only clean". I dunno. > 2) change the behavior of -x/-X This won't happen without a long deprecation period. If you haven't seen and read them, check the recent list archive for the past few weeks, with keywords "trashable", "precious", etc.
Re: [PATCH] rebase -i: introduce the 'test' command
On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: > > > Would it not make more sense to add a command-line option (and a config > > > setting) to re-schedule failed `exec` commands? Like so: > > > > Your proposition would do in most cases, however it is not possible to > > make a distinction between reschedulable and non-reschedulable commands. > > True. But I don't think that's so terrible. > > What I think is something to avoid is two commands that do something very, > very similar, but with two very, very different names. > > 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. I wonder how often this kind of "yes, I know it fails, but keep going anyway" situation would come up. And what the interface is like for getting past it. E.g., what if you fixed a bunch of stuff but your tests still fail? You may not want to abandon the changes you've made, but you need to "rebase --continue" to move forward. I encounter this often when the correct fix is actually in an earlier commit than the one that yields the test failure. You can't rewind an interactive rebase, so I complete and restart it, adding an "e"dit at the earlier commit. How would I move past the test that fails to continue? I guess "git rebase --edit-todo" and then manually remove it (and any other remaining test lines)? That's not too bad, but I wonder if people would find it more awkward than the current way (which is to just "rebase --continue" until you get to the end). I dunno. I am not sure if I am for or against changing the default, so these are just some musings. :) -Peff
Re: [PATCH] t5562: skip if NO_CURL is enabled
On Wed, Nov 28, 2018 at 03:56:29PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Thu, Nov 22 2018, Jeff King wrote:
>
> > On Thu, Nov 22, 2018 at 02:17:01AM -0800, Carlo Arenas wrote:
> >> PS. upstreaming the PERL_PATH fix is likely to be good to do soonish
> >> as I presume at least all BSD might be affected, let me know if you
> >> would rather me do that instead as I suspect we might be deadlocked
> >> otherwise ;)
> >
> > Yeah, the $PERL_PATH thing is totally orthogonal, and should graduate
> > separately.
>
> On the subject of orthagonal things: This test fails on AIX with /bin/sh
> (but not /bin/bash) due to some interaction of ssize_b100dots and the
> build_option function. On that system:
>
> $ ./git version --build-options
> git version 2.20.0-rc1
> cpu: 00FA74164C00
> no commit associated with this build
> sizeof-long: 4
> sizeof-size_t: 4
>
> But it somehow ends up in the 'die' condition in that case statement. I
> dug around briefly but couldn't find the cause, probably some limitation
> in the shell constructs it supports. Just leaving a note about this...
That's weird. The functions involved are pretty vanilla. I'd suspect
something funny with the sed invocation:
build_option () {
git version --build-options |
sed -ne "s/^$1: //p"
}
but that's the one thing that shouldn't be dependent on the shell in
use.
Can you manually replicate the shell commands to see where it goes
wrong?
-Peff
Re: [PATCH] t5562: skip if NO_CURL is enabled
On Wed, Nov 28, 2018 at 02:27:08PM +0100, SZEDER Gábor wrote:
> > Curiously, the act.err file also has 54 NUL bytes before the "fatal:"
> > message.
>
> I think those NUL bytes come from the file system.
>
> The contents of 'act.err' from the previous test ('fetch gzipped
> empty') is usually:
>
> fatal: request ended in the middle of the gzip stream
> fatal: the remote end hung up unexpectedly
>
> Notice that the length of the first line is 54 bytes (including the
> trailing newline). So I suspect that the following is happening:
>
> - http-backend in the previous test writes the first line,
> - that test finishes and this one starts,
> - this test truncates 'act.err',
> - and then the still-running http-backend from the previous test
> finally writes the second line.
>
> So at this point 'act.err' is empty, but the offset of the fd of the
> redirection still open from the previous test is at 54, so the file
> system fills those bytes with NULs.
Right, good thinking. Thanks for the explanation!
-Peff
Re: [PATCH 8/9] sha1-file: use loose object cache for quick existence check
On Tue, Nov 27, 2018 at 09:48:57PM +0100, René Scharfe wrote:
> > +static int quick_has_loose(struct repository *r,
> > + const unsigned char *sha1)
> > +{
> > + int subdir_nr = sha1[0];
> > + struct object_id oid;
> > + struct object_directory *odb;
> > +
> > + hashcpy(oid.hash, sha1);
> > +
> > + prepare_alt_odb(r);
> > + for (odb = r->objects->odb; odb; odb = odb->next) {
> > + odb_load_loose_cache(odb, subdir_nr);
>
> Is this thread-safe? What happens if e.g. one index-pack thread resizes
> the array while another one sorts it?
No, but neither is any of the object_info / has_object_file path, which
may use static function-local buffers, or (before my series) alt scratch
bufs, or even call reprepare_packed_git().
In the long run, I think the solution is probably going to be pushing
some mutexes into the right places, and putting one around the cache
fill is an obvious place.
> Loading the cache explicitly up-front would avoid that, and improves
> performance a bit in my (very limited) tests on an SSD. Demo patch for
> next at the bottom. How does it do against your test cases?
It's going to do badly on corner cases where we don't need to load every
object subdirectory, and one or more of them are big. I.e., if I look up
"1234abcd", the current code only needs to fault in $GIT_DIR/objects/12.
Pre-loading means we'd hit them all. Even without a lot of objects, on
NFS that's 256 latencies instead of 1.
-Peff
Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed
On Fri, Nov 30, 2018 at 05:32:47PM -0800, Matthew DeVore wrote: > > Speaking of which, would this flag work better as a field in > > setup_revision_opt, which is passed to setup_revisions()? The intent > > seem to be to influence how we parse command-line arguments, and that's > > where other similar flags are (e.g., assume_dashdash). > > Good idea. This would solve the problem of mistakenly believing the flag > matters when it doesn't, since it is in the struct which is used as the > arguments of the exact function that cares about it. Here's a new patch - > I'm tweaking e-mail client settings so hopefully this makes it to the list > without mangling - if not I'll resend it with `git-send-email` later. > > From 941c89fe1e226ed4d210ce35d0d906526b8277ed Mon Sep 17 00:00:00 2001 > From: Matthew DeVore > Date: Fri, 30 Nov 2018 16:43:32 -0800 > Subject: [PATCH] revisions.c: put promisor option in specialized struct > > Put the allow_exclude_promisor_objects flag in setup_revision_opt. When > it was in rev_info, it was unclear when it was used, since rev_info is > passed to functions that don't use the flag. This resulted in > unnecessary setting of the flag in prune.c, so fix that as well. Thanks, this looks pretty reasonable overall. Two comments: > repo_init_revisions(the_repository, , NULL); > save_commit_buffer = 0; > - revs.allow_exclude_promisor_objects_opt = 1; > - setup_revisions(ac, av, , NULL); > + > + memset(_r_opt, 0, sizeof(s_r_opt)); > + s_r_opt.allow_exclude_promisor_objects = 1; > + setup_revisions(ac, av, , _r_opt); I wonder if a static initializer for setup_revision_opt is worth it. It would remove the need for this memset. Probably not a big deal either way, though. > static int handle_revision_opt(struct rev_info *revs, int argc, const char > **argv, > -int *unkc, const char **unkv) > +int *unkc, const char **unkv, > +int allow_exclude_promisor_objects) Why not pass in the whole setup_revision_opt struct? We don't need anything else from it yet, but it seems like the point of that struct is to pass around preferences like this. -Peff
Re: [PATCH] Do not fail test if '.' is part of $PATH
On Sat, Dec 01, 2018 at 06:07:57PM +0100, H.Merijn Brand wrote: > When $PATH contains the current directory as .:PATH, PATH:., PATH:.:PATH, > or (maybe worse) as :PATH, PATH:, or PATH::PATH - as an empty entry is > identical to having dot in $PATH - this test used to fail Good catch. The test cares about Git not accidentally adding "." to the PATH, but we can't check that if it is already there. > This patch was tested with PATH=$PATH, PATH=.:$PATH, PATH=$PATH:., > PATH=$PATH:.:/bin, PATH=:$PATH, PATH=$PATH:, and PATH=$PATH::/bin > [...] > +test_lazy_prereq DOT_IN_PATH ' > + case ":$PATH:" in > + *:.:*|*::*) true ;; > + *) false ;; > + esac > +' Since the test is ultimately checking "can we run should-not-run from the current directory", might it be simpler to actually try that as the precondition? I.e., something like: test_expect_success 'create program in current directory' ' write_script should-not-run <<-\EOF && echo yikes EOF ' test_lazy_prereq DOT_IN_PATH ' should-not-run ' test_expect_success !DOT_IN_PATH 'run_command is restricted to PATH' ' test_must_fail test-tool run-command run-command should-not-run ' ? That's more lines, but we don't have to peek into the details of how $PATH works. -Peff
Re: [PATCH] t6036: avoid "cp -a"
Thanks both. Agree with Junio it would be better if squashed; apologize
for not catching it earlier, but the following might help to make it
visible for anyone that care to run the linter:
$ make test-lint-shell-syntax
Carlo
-- >8 --
From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?=
Subject: [PATCH] tests: add lint for non portable cp -a
cp -a, while a common flag isn't in POSIX and will therefore fail
on systems that don't have GNUish tools (like OpenBSD, AIX or Solaris)
Signed-off-by: Carlo Marcelo Arenas Belón
---
t/check-non-portable-shell.pl | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b45bdac688..8037eef777 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -35,6 +35,7 @@ sub err {
chomp;
}
+ /\bcp\s+-a/ and err 'cp -a is not portable';
/\bsed\s+-i/ and err 'sed -i is not portable';
/\becho\s+-[neE]/ and err 'echo with option is not portable (use
printf)';
/^\s*declare\s+/ and err 'arrays/declare not portable';
--
2.20.0.rc2
Re: [PATCH] t6036: avoid "cp -a"
Elijah Newren writes:
> Thanks for the patch!
>
> On Fri, Nov 30, 2018 at 6:52 PM Carlo Marcelo Arenas Belón
> ...
> Oops. Thanks for catching. To be honest, we don't even need -a, -R,
> etc. -- it was just a habit for me to add -a after cp. A simple cp
> would do, though what you have here is fine too.
Thanks, both. I think the topic won't escape 'next' until 2.20
final, and after that we can rewind 'next', so it may be better
to squash it in, but anyway...
-- >8 --
From: Carlo Marcelo Arenas Belón
Date: Fri, 30 Nov 2018 18:52:12 -0800
Subject: [PATCH] t6036: avoid non-portable "cp -a"
b8cd1bb713 ("t6036, t6043: increase code coverage for file collision
handling", 2018-11-07) uses this GNU extension that is not available
in a POSIX complaint cp. In this particular case, there is no need to
use the option, as it is just copying a single file to create another
file.
Signed-off-by: Carlo Marcelo Arenas Belón
Signed-off-by: Junio C Hamano
---
t/t6036-recursive-corner-cases.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t6036-recursive-corner-cases.sh
b/t/t6036-recursive-corner-cases.sh
index b7488b00c0..d23b948f27 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -1631,7 +1631,7 @@ test_expect_success 'check nested conflicts' '
# Compare m to expected contents
>empty &&
- cp -a m_stage_2 expected_final_m &&
+ cp m_stage_2 expected_final_m &&
test_must_fail git merge-file --diff3 \
-L "HEAD" \
-L "merged common ancestors" \
--
2.20.0-rc1-10-g7068cbc4ab
Re: [RFC PATCH] Introduce "precious" file concept
On Wed, Nov 28, 2018 at 10:54 PM Ævar Arnfjörð Bjarmason wrote: > But we must have some viable way to repair warts in the tools, and > losing user data is a *big* wart. > > I don't think something like the endgame you've described in > https://public-inbox.org/git/xmqqzhtwuhpc@gitster-ct.c.googlers.com/ > is ever going to work. Novice git users (the vast majority) are not > going to diligently update both .gitignore and some .gitattribute > mechanism in lockstep. I'd bet most git users haven't read more than a > few paragraphs of our entire documentation at best. > > So what's the way forward? I think ultimately we must move to something > where we effectively version the entire CLI UI similar to stable API > versions. I.e. for things like this that would break some things (or > Duy's new "split checkout") introduce them as flags first, then bundle > up all such flags and cut a major release "Git 3, 4, ...", and > eventually remove old functionality. Related to Duy's new "split chekckout", I just realized that I added --overwrite-ignore (enabled by default) [1] years ago to allow to out out of this behavior. We could turn --no-overwrite-ignore by default on the new command "git switch-branch" to err on the safe side. Then the user could switch to --overwrite-ignore once they learn more about gitignore and gitattributes (or the coming "backup log"). I'm not sure if I really like this, but at least it's one of the options. [1] https://public-inbox.org/git/1322388933-6284-2-git-send-email-pclo...@gmail.com/ -- Duy
Re: [PATCH] t6036: avoid "cp -a"
Hi,
Thanks for the patch!
On Fri, Nov 30, 2018 at 6:52 PM Carlo Marcelo Arenas Belón
wrote:
>
> b8cd1bb713 ("t6036, t6043: increase code coverage for file collision
> handling", 2018-11-07) uses this GNU extension that is not available in a
> POSIX complaint
This is an extraordinarily long line; can you rewrap at around 72 characters?
> cp; use cp -R instead
>
> Signed-off-by: Carlo Marcelo Arenas Belón
> ---
> to be applied on top of en/merge-path-collision for next
>
> t/t6036-recursive-corner-cases.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t6036-recursive-corner-cases.sh
> b/t/t6036-recursive-corner-cases.sh
> index b7488b00c0..fdb120d0dc 100755
> --- a/t/t6036-recursive-corner-cases.sh
> +++ b/t/t6036-recursive-corner-cases.sh
> @@ -1631,7 +1631,7 @@ test_expect_success 'check nested conflicts' '
>
> # Compare m to expected contents
> >empty &&
> - cp -a m_stage_2 expected_final_m &&
> + cp -R m_stage_2 expected_final_m &&
> test_must_fail git merge-file --diff3 \
> -L "HEAD" \
> -L "merged common ancestors" \
> --
> 2.20.0.rc1.6.ga1598010f
Oops. Thanks for catching. To be honest, we don't even need -a, -R,
etc. -- it was just a habit for me to add -a after cp. A simple cp
would do, though what you have here is fine too.
Re: Parsing a git HTTP protocol response
On Fri, Nov 30, 2018 at 6:58 PM Bryan Turner wrote: > > Here's a (very ugly) patch I threw together on top of your code: ...snip Gmail butchered my patch, so here it is as an attachment. Bryan short-size-reads.patch Description: Binary data
Re: Parsing a git HTTP protocol response
On Fri, Nov 30, 2018 at 5:05 PM Farhan Khan wrote:
>
> Hi all,
>
> I am writing an implementation of the git HTTP pack protocol in C. It
> just does a request to clone a repository. It works pretty well for
> small repositories, but seems to fail on larger repositories and I do
> not understand why.
>
> All that my code does is send a hard-coded "want" request. As I
> understand it, responses come with a four-byte size prefix, then the
> data of the size - 4. The first four bytes are the size of the object
> in ASCII and after that size, a new object should begin by specifying
> its size. The final "" string should signify the end.
>
> On small repositories my code works fine. But when use a large git
> repository, I hit an object size that cannot be interpreted by ASCII
> into a number. In particular, right before the crash I am pretty
> consistently getting a size starting with 0x32,0x00 or 0x32,0x30,0x00
> (note, it starts with 0x32 and has 0x00 in there). This is not a
> readable four byte ascii value, likely an erroneous size value, which
> causes the next object size calculation to land incorrectly and
> subsequently the program to malfunction.
>
> My questions:
> 1. Am I misunderstanding the protocol?
> 2. Does 0x32 signify something?
> 3. Also, where can I see in the source code git parses the data it
> receives from a "want" request?
>
> If you would like to see my code, it is located here:
> http://git.farhan.codes/farhan/post
> To compile to Linux run: gcc post.c main.c -lcurl -o post
> To compile on FreeBSD you can use the Makefile or: cc -L
> /usr/local/lib -I /usr/local/include -lcurl main.c post.c -o post
> In both cases you need to have libcurl installed.
>
> To run do: ./post [a git http url] [a commit value]
> ie: ./post http://git.farhan.codes/farhan/openbsd
> 373dd5ba116d42cddf1fdcb58b578a4274f6d589
>
> I have a lot of debugging printf() messages, but it eventually hits a
> point where you see this:
>
> Start=
> Current stats: Current state: 999 Received: 1448
> We have enough bytes, moving forward
> == New Object
> No Error, object is 32 bytes
> Size bytes: 32 30 00 00
I modified your debugging output a little bit, and right before the
failure happens I see this in my output:
Start=
Current stats: Current state: 999 Received: 1298
Read complete; still missing 1296 bytes (6901 read of 8197)
Offset: 1298
Start=
Current stats: Current state: 999 Received: 1298
Read complete; 2 more bytes in buffer
== New Object
Size bytes: 32 30 00 2b
Based on that, it appears what's happening is you're not handling the
case where you end up with fewer than 4 bytes in the buffer. As a
result, memcpy reads past the end of the response buffer and gets
whatever it gets, resulting in an incorrect parsed size.
The while loop in pack_protocol_line is checking +1, but I think it
should be checking +3 to ensure there's at least 4 bytes so it can get
a complete size. The "parseread" struct will need to grow a couple
more fields to allow buffering some additional bytes between
pack_protocol_line calls (because curl requires the write function to
always consume the full buffer) and, at the start of the loop, when
reading/parsing a size, the code will need to consider any buffered
bytes from the previous function call. That requires some knock-on
changes to how the offset is handled, as well as the the initial
"psize" set when starting a new packet.
Once you start accounting for reads that cut off in 1, 2 or 3 bytes
into the 4 required to parse a size, your program should be able to
run to completion.
Here's a (very ugly) patch I threw together on top of your code:
diff --git a/main.c b/main.c
index 956362b..8873fd5 100644
--- a/main.c
+++ b/main.c
@@ -71,7 +71,7 @@ int main(int argc, char *argv[]) {
curl_easy_setopt(curl, CURLOPT_HTTPHEADER, list);
curl_easy_setopt(curl, CURLOPT_POSTFIELDSIZE, (long)content_length);
curl_easy_setopt(curl, CURLOPT_WRITEDATA, );
- curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, pack_protocol_line);
+ curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, _protocol_line);
res = curl_easy_perform(curl);
if (curl != CURLE_OK)
diff --git a/post.c b/post.c
index cfffd5c..4f8512c 100644
--- a/post.c
+++ b/post.c
@@ -36,89 +36,83 @@ size_t pack_protocol_line(void *buffer, size_t
size, size_t nmemb, void *userp)
{
unsigned char *reply = buffer;
struct parseread *parseread = userp;
- int offset = 0;
+ int offset = 0, pending = 0, remaining = 0;
char tmp[5];
- int check;
-
- int pool = size * nmemb;
printf("\n\nStart=\n");
- printf("Current stats: Current state: %d Received: %ld\n",
parseread->state, size*nmemb);
+ printf("Current stats: Current state: %d Received: %ld\n",
parseread->state, nmemb);
- while(offset + 1 < size + nmemb) {
+ while (offset + 3 < nmemb) {
if (parseread->state == STATE_NEWLINE) {
printf("== New Object\n");
bzero(tmp, 5);
- memcpy(tmp, buffer+offset, 4); tmp[4] = '\0';
- check
Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed
On 11/21/2018 08:40 AM, Jeff King wrote:
On Mon, Oct 22, 2018 at 06:13:42PM -0700, Matthew DeVore wrote:
diff --git a/builtin/prune.c b/builtin/prune.c
index 41230f8215..11284d0bf3 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -120,6 +120,7 @@ int cmd_prune(int argc, const char **argv, const char
*prefix)
save_commit_buffer = 0;
read_replace_refs = 0;
ref_paranoia = 1;
+ revs.allow_exclude_promisor_objects_opt = 1;
repo_init_revisions(the_repository, , prefix);
argc = parse_options(argc, argv, prefix, options, prune_usage, 0);
I think this line is in the wrong place. The very first thing
repo_init_revisions() will do is memset() the revs struct to all-zeroes,
so it cannot possibly be doing anything.
Ah of course :)
Normally it would need to go after init_revisions() but before
setup_revisions(), but we don't seem to call the latter at all in
builtin/prune.c. Which makes sense, because you cannot pass options to
influence the reachability traversal. So I don't think we need to care
about this flag at all here.
Agreed, prune.c doesn't use setup_revisions() even transitively, so we
don't care about this flag.
Speaking of which, would this flag work better as a field in
setup_revision_opt, which is passed to setup_revisions()? The intent
seem to be to influence how we parse command-line arguments, and that's
where other similar flags are (e.g., assume_dashdash).
Good idea. This would solve the problem of mistakenly believing the flag
matters when it doesn't, since it is in the struct which is used as the
arguments of the exact function that cares about it. Here's a new patch
- I'm tweaking e-mail client settings so hopefully this makes it to the
list without mangling - if not I'll resend it with `git-send-email` later.
From 941c89fe1e226ed4d210ce35d0d906526b8277ed Mon Sep 17 00:00:00 2001
From: Matthew DeVore
Date: Fri, 30 Nov 2018 16:43:32 -0800
Subject: [PATCH] revisions.c: put promisor option in specialized struct
Put the allow_exclude_promisor_objects flag in setup_revision_opt. When
it was in rev_info, it was unclear when it was used, since rev_info is
passed to functions that don't use the flag. This resulted in
unnecessary setting of the flag in prune.c, so fix that as well.
Signed-off-by: Matthew DeVore
---
builtin/pack-objects.c | 7 +--
builtin/prune.c| 1 -
builtin/rev-list.c | 6 --
revision.c | 17 -
revision.h | 4 ++--
5 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 24bba8147f..b22c99f540 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3084,14 +3084,17 @@ static void record_recent_commit(struct commit
*commit, void *data)
static void get_object_list(int ac, const char **av)
{
struct rev_info revs;
+ struct setup_revision_opt s_r_opt;
char line[1000];
int flags = 0;
int save_warning;
repo_init_revisions(the_repository, , NULL);
save_commit_buffer = 0;
- revs.allow_exclude_promisor_objects_opt = 1;
- setup_revisions(ac, av, , NULL);
+
+ memset(_r_opt, 0, sizeof(s_r_opt));
+ s_r_opt.allow_exclude_promisor_objects = 1;
+ setup_revisions(ac, av, , _r_opt);
/* make sure shallows are read */
is_repository_shallow(the_repository);
diff --git a/builtin/prune.c b/builtin/prune.c
index e42653b99c..1ec9ddd751 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -120,7 +120,6 @@ int cmd_prune(int argc, const char **argv, const
char *prefix)
save_commit_buffer = 0;
read_replace_refs = 0;
ref_paranoia = 1;
- revs.allow_exclude_promisor_objects_opt = 1;
repo_init_revisions(the_repository, , prefix);
argc = parse_options(argc, argv, prefix, options, prune_usage, 0);
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 3a2c0c23b6..c3095c6fed 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -362,6 +362,7 @@ int cmd_rev_list(int argc, const char **argv, const
char *prefix)
{
struct rev_info revs;
struct rev_list_info info;
+ struct setup_revision_opt s_r_opt;
int i;
int bisect_list = 0;
int bisect_show_vars = 0;
@@ -375,7 +376,6 @@ int cmd_rev_list(int argc, const char **argv, const
char *prefix)
git_config(git_default_config, NULL);
repo_init_revisions(the_repository, , prefix);
revs.abbrev = DEFAULT_ABBREV;
- revs.allow_exclude_promisor_objects_opt = 1;
revs.commit_format = CMIT_FMT_UNSPECIFIED;
revs.do_not_die_on_missing_tree = 1;
@@ -407,7 +407,9 @@ int cmd_rev_list(int argc, const char **argv, const
char *prefix)
}
}
- argc = setup_revisions(argc, argv, , NULL);
+ memset(_r_opt, 0, sizeof(s_r_opt));
+ s_r_opt.allow_exclude_promisor_objects = 1;
+ argc =
Re: [PATCH v3 06/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
Hi,
On Fri, 30 Nov 2018, Phillip Wood wrote:
> > diff --git a/sequencer.c b/sequencer.c
> > index 900899ef20..11692d0b98 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -4394,24 +4394,29 @@ int sequencer_make_script(FILE *out, int argc, const
> > char **argv,
> > return 0;
> > }
> >
> > -/*
> > - * Add commands after pick and (series of) squash/fixup commands
> > - * in the todo list.
> > - */
> > -int sequencer_add_exec_commands(const char *commands)
> > +static void todo_list_add_exec_commands(struct todo_list *todo_list,
> > + struct string_list *commands)
> > {
> > - const char *todo_file = rebase_path_todo();
> > - struct todo_list todo_list = TODO_LIST_INIT;
> > - struct strbuf *buf = _list.buf;
> > - size_t offset = 0, commands_len = strlen(commands);
> > - int i, insert;
> > + struct strbuf *buf = _list->buf;
> > + const char *old_buf = buf->buf;
> > + size_t base_length = buf->len;
> > + int i, insert, nr = 0, alloc = 0;
> > + struct todo_item *items = NULL, *base_items = NULL;
> >
> > - if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
> > - return error(_("could not read '%s'."), todo_file);
> > + for (i = 0; i < commands->nr; ++i) {
> > + strbuf_addstr(buf, commands->items[i].string);
> > + strbuf_addch(buf, '\n');
> > + }
>
> This could cause buf->buf to be reallocated in which case all the
> todo_list_item.arg pointers will be invalidated.
I guess it is a good time for me to admit that the `arg` idea was not my
best. Maybe it would be good to convert that from a pointer to an offset,
e.g. `size_t arg_offset_in_buf;`? Or maybe we should just drop the
`_in_buf` suffix and clarify in a comment next to the declaration of the
fields that they are offsets in the strbuf?
Ciao,
Dscho
