* Re: sparse checkout - weird behavior
From: Jeff King @ 2017-01-26 4:59 UTC (permalink / raw)
To: Paul Hammant; +Cc: git
In-Reply-To: <CA+298Ujx2wH2WnoYiOaWKoneBrF_E5VUXXSMqecGgNLYS0Wemg@mail.gmail.com>
On Wed, Jan 25, 2017 at 10:21:19PM -0500, Paul Hammant wrote:
> Related bug (maybe the same). Reproduction:
>
> $ git clone git@github.com:jekyll/jekyll.git --no-checkout
> Cloning into 'jekyll'...
> remote: Counting objects: 41331, done.
> remote: Compressing objects: 100% (5/5), done.
> remote: Total 41331 (delta 0), reused 0 (delta 0), pack-reused 41326
> Receiving objects: 100% (41331/41331), 11.91 MiB | 9.15 MiB/s, done.
> Resolving deltas: 100% (26530/26530), done.
> $ cd jekyll
> $ git config core.sparsecheckout true
> $ echo 'CONDUCT.markdown' > .git/info/sparse-checkout
> $ echo 'Gemfile' >> .git/info/sparse-checkout
> $ echo 'Rakefile' >> .git/info/sparse-checkout
> $ echo 'appveyor.yml' >> .git/info/sparse-checkout
> $ git checkout --
> Your branch is up-to-date with 'origin/master'.
> $ ls
> CONDUCT.markdown Gemfile Rakefile appveyor.yml lib
>
> I was not expecting to see 'lib' in the resulting file list
Yep, I think this is the same problem. Inside lib, you get only
"lib/theme_template/Gemfile", because it matches your unanchored
pattern. Using "/Gemfile" in the sparse-checkout file fixes it.
-Peff
^ permalink raw reply
* Re: Force Confirmation for Dropping Changed Lines
From: Jacob Keller @ 2017-01-26 5:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Hilco Wijbenga, Git Users
In-Reply-To: <xmqqd1fa7dqf.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 25, 2017 at 6:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Where did you get that "unset" from? If that is this paragraph in
> Documentation/gitattributes.txt:
>
Ok so that whole section of documentation is very confusing to me.
Maybe it could be improved for more readability. I'll see if I can't
help clear up some of my confusion.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH] gpg-interface: Add some output from gpg when it errors out.
From: Jeff King @ 2017-01-26 5:21 UTC (permalink / raw)
To: Mike Hommey; +Cc: Junio C Hamano, git
In-Reply-To: <20170125235410.byxwmo7o7zdszzot@glandium.org>
On Thu, Jan 26, 2017 at 08:54:10AM +0900, Mike Hommey wrote:
> > Implementation-wise, I'd be happier if we do not add any new
> > callsites of strbuf_split(), which is a horrible interface. But
> > that is a minor detail.
>
> What would you suggest otherwise?
Try string_list_split() (or its in_place() variant, since it is probably
OK to hack up the string for your use case). Like this:
diff --git a/gpg-interface.c b/gpg-interface.c
index 2768bb307..051bb7d3e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -158,14 +158,16 @@ static int pipe_gpg_command(struct child_process *cmd,
/* Print out any line that doesn't start with [GNUPG:] if the gpg
* command failed. */
if (ret) {
- struct strbuf **err_lines = strbuf_split(err, '\n');
- for (struct strbuf **line = err_lines; *line; line++) {
- if (memcmp((*line)->buf, "[GNUPG:]", 8)) {
- strbuf_rtrim(*line);
- fprintf(stderr, "%s\n", (*line)->buf);
- }
+ struct string_list lines = STRING_LIST_INIT_NODUP;
+ int i;
+
+ string_list_split_in_place(&lines, err->buf, '\n', 0);
+ for (i = 0; i < lines.nr; i++) {
+ const char *line = lines.items[i].string;
+ if (!starts_with(line, "[GNUPG:]"))
+ fprintf(stderr, "%s\n", line);
}
- strbuf_list_free(err_lines);
+ string_list_clear(&lines, 0);
}
return ret;
}
Note that I also replaced the memcmp with starts_with(). That avoids the
magic number "8". I also suspect your original can read off the end of
the buffer when the line is shorter than 8 characters (e.g., if memcmp
does 64-bit loads).
-Peff
^ permalink raw reply related
* Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Johannes Sixt @ 2017-01-26 6:39 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Johannes Schindelin, David Aguilar, Ramsay Jones,
GIT Mailing-list
In-Reply-To: <20170125220101.et67ebkumsqosaku@sigill.intra.peff.net>
Am 25.01.2017 um 23:01 schrieb Jeff King:
> +#pragma GCC diagnostic ignored "-Wformat-zero-length"
Last time I used #pragma GCC in a cross-platform project, it triggered
an "unknown pragma" warning for MSVC. (It was the C++ compiler, I don't
know if the C compiler would also warn.) It would have to be spelled
like this:
#pragma warning(disable: 4068) /* MSVC: unknown pragma */
#pragma GCC diagnostic ignored "-Wformat-zero-length"
Dscho mentioned that he's compiling with MSVC. It would do him a favor.
-- Hannes
^ permalink raw reply
* Re: [PATCH] Documentation: implement linkgit macro for Asciidoctor
From: Eric Wong @ 2017-01-26 7:43 UTC (permalink / raw)
To: Jeff King; +Cc: brian m. carlson, git, Johannes Schindelin, Øyvind A. Holm
In-Reply-To: <20170126034655.fwzow2mgkjj5dpek@sigill.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> On Thu, Jan 26, 2017 at 12:13:44AM +0000, brian m. carlson wrote:
> > +
> > + def process(parent, target, attrs)
> > + if parent.document.basebackend? 'html'
> > + prefix = parent.document.attr('git-relative-html-prefix')
> > + %(<a href="#{prefix}#{target}.html">#{target}(#{attrs[1]})</a>\n)
> > + elsif parent.document.basebackend? 'docbook'
> > + %(<citerefentry>
> > +<refentrytitle>#{target}</refentrytitle><manvolnum>#{attrs[1]}</manvolnum>
> > +</citerefentry>
> > +)
<snip>
> The multi-line string is kind of ugly because of the indentation.
> Apparently Ruby has here-docs that will eat leading whitespace, but the
> syntax was not introduce until Ruby 2.3, which is probably more recent
> than we should count on.
You can use '\' to continue long lines with any Ruby version:
"<citerefentry>" \
"<refentrytitle>#{target}</refentrytitle>" \
"<manvolnum>#{attrs[1]}</manvolnum>" \
"</citerefentry>"
The above happens during the parse phase, so there's no garbage
or method call overhead compared to the more-frequently seen '+'
or '<<' method calls to combine strings.
> I think you could write:
>
> %(<citerefentry>
> <refentrytitle>#{target}</refentrytitle><manvolnum>#{attrs[1]}</manvolnum>
> </citerefentry>
> ).gsub(/^\s*/, "")
>
> I don't know if that's too clever or not.
Ick...
> But either way, I like this better than introducing an extra dependency.
Agreed.
^ permalink raw reply
* Re: [PATCH 2/5] revision.c: group ref selection options together
From: Duy Nguyen @ 2017-01-26 9:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Jeff King, Jacob Keller
In-Reply-To: <xmqqo9yualqn.fsf@gitster.mtv.corp.google.com>
On Thu, Jan 26, 2017 at 4:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> * I am expecting that the new one yet to be introduced will not
> share the huge "switch (selector)" part, but does its own things
> in a separate function with a similar structure. The only thing
> common between these two functions would be the structure
> (i.e. it has a big "switch(selector)" that does different things
> depending on REF_SELECT_*) and a call to clear_* function.
Yep. The "new one" is demonstrated in 5/5.
> If we were to add a new kind of REF_SELECT_* (say
> REF_SELECT_NOTES just for the sake of being concrete), what
> changes will be needed to the code if the addition of "use reflog
> from this class of refs for decoration" feature was done with or
> without this step? I have a suspicion that the change will be
> simpler without this step.
The switch/case is to deal with new REF_SELECT_* (at least it's how I
imagine it). What I was worried about was, when a user adds
--select-notes, they may not be aware that it's in the same
all/branches/tags/remotes group that's supposed to work with
--decorate-reflog as well, and as a result "--decorate-reflog
--select-notes" is the same as "--select-notes".
With the switch/case, when you add a new enum item, at the least the
compiler should warn about unhandled cases. And we can have a new
"case REF_SELECT_NOTES:" for both --exclude and --decorate-reflog.
Without the switch/case, I guess it's still possible to do something
like
if (!strcmp(arg, "--select-notes")) {
if (preceded_by_exclude())
does_one_thing();
else if (preceded_by_decorate_reflog())
does_another_thing();
}
It's probably easier to maintain though, if all
decorate-reflog-related things are grouped together, rather than
spread out per option like the above.
--
Duy
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
From: Lars Schneider @ 2017-01-26 9:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git
In-Reply-To: <xmqq7f5i92jk.fsf@gitster.mtv.corp.google.com>
> On 25 Jan 2017, at 23:51, Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
>> I guess the way to dig would be to add a test that looks at the output
>> of "type mv" or something, push it to a Travis-hooked branch, and then
>> wait for the output
>
> Sounds tempting ;-)
Well, I tried that:
mv is /bin/mv
... and "/bin/mv" is exactly the version that I have on my machine.
The difference between Travis and my machine is that I changed the
default shell to ZSH with a few plugins [1]. If I run the test with
plain BASH on my Mac then I can reproduce the test failure. Therefore,
we might want to adjust the commit message if anyone else can reproduce
the problem on a Mac.
I can even reproduce the failure if I run the test with plain ZSH.
However, I can't find a plugin that defines an alias for "mv". Puzzled...
- Lars
[1] https://github.com/robbyrussell/oh-my-zsh
^ permalink raw reply
* Re: [PATCH 2/5] revision.c: group ref selection options together
From: Duy Nguyen @ 2017-01-26 9:18 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Jacob Keller
In-Reply-To: <20170125205037.cg3aebh5rsx4zb2l@sigill.intra.peff.net>
On Thu, Jan 26, 2017 at 3:50 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 25, 2017 at 07:50:51PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> These options have on thing in common: when specified right after
>> --exclude, they will de-select refs instead of selecting them by
>> default.
>>
>> This change makes it possible to introduce new options that use these
>> options in the same way as --exclude. Such an option would just
>> implement something like handle_refs_pseudo_opt().
>>
>> parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so
>> that similar functions like handle_refs_pseudo_opt() are forced to
>> handle all ref selector options, not skipping some by mistake, which may
>> revert the option back to default behavior (rev selection).
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> revision.c | 134 +++++++++++++++++++++++++++++++++++++++++++++----------------
>> 1 file changed, 100 insertions(+), 34 deletions(-)
>
> Hmm. I see what you're trying to do here, and abstract the repeated
> bits. But I'm not sure the line-count reflects a real simplification.
> Everything ends up converted to an enum, and then that enum just expands
> to similar C code.
It's not simplification, but hopefully for better maintainability. This
if (strcmp(arg, "--remotes")) {
if (preceded_by_exclide())
does_something();
else if (preceded_by_decorate())
does_another()
} else if (strcmp(arg, "--branches")) {
if (preceded_by_exclide())
does_something();
else if (preceded_by_decorate())
does_another()
}
starts to look ugly especially when the third "preceded_by_" comes
into picture. Putting all "does_something" in one group and
"does_another" in another, I think, gives us a better view how ref
selection is handled for a specific operation like --exclude or
--decorate-ref.
> I kind of expected that clear_ref_exclusion() would just become a more
> abstract clear_ref_selection(). For now it would clear exclusions, and
> then later learn to clear the decoration flags.
It may go that way, depending on how we handle these options for
decorate-reflog. The current load_ref_decorations() is not really
suited for fine-grained ref selection yet.
--
Duy
^ permalink raw reply
* Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude
From: Duy Nguyen @ 2017-01-26 9:28 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Jacob Keller
In-Reply-To: <20170125205718.ksqstdnazmgbkehy@sigill.intra.peff.net>
On Thu, Jan 26, 2017 at 3:57 AM, Jeff King <peff@peff.net> wrote:
> I don't think it means either. It means to include remotes in the
> selected revisions, but excluding the entries mentioned by --exclude.
>
> IOW:
>
> --exclude=foo --remotes
> include all remotes except refs/remotes/foo
>
> --exclude=foo --unrelated --remotes
> same
>
> --exclude=foo --decorate-reflog --remotes
> decorate reflogs of all remotes except "foo". Do _not_ use them
> as traversal tips.
>
> --decorate-reflog --exclude=foo --remotes
> same
>
> IOW, the ref-selector options build up until a group option is given,
> which acts on the built-up options (over that group) and then resets the
> built-up options. Doing "--unrelated" as above is orthogonal (though I
> think in practice nobody would do that, because it's hard to read).
This is because it makes sense to combine --exclude and
--decorate-reflog. But what about a new --something that conflicts
with either --exclude or --decorate-reflog? Should we simply catch
such combinations and error out (which may be a bit more complicated
than this patch, or maybe not)?
--
Duy
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
From: Lars Schneider @ 2017-01-26 9:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git
In-Reply-To: <D9F0976B-9F78-44BE-B9DD-CAB6506FA3A9@gmail.com>
> On 26 Jan 2017, at 10:14, Lars Schneider <larsxschneider@gmail.com> wrote:
>
>
>> On 25 Jan 2017, at 23:51, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Jeff King <peff@peff.net> writes:
>>
>>> I guess the way to dig would be to add a test that looks at the output
>>> of "type mv" or something, push it to a Travis-hooked branch, and then
>>> wait for the output
>>
>> Sounds tempting ;-)
>
> Well, I tried that:
>
> mv is /bin/mv
>
> ... and "/bin/mv" is exactly the version that I have on my machine.
>
> The difference between Travis and my machine is that I changed the
> default shell to ZSH with a few plugins [1]. If I run the test with
> plain BASH on my Mac then I can reproduce the test failure. Therefore,
> we might want to adjust the commit message if anyone else can reproduce
> the problem on a Mac.
>
> I can even reproduce the failure if I run the test with plain ZSH.
> However, I can't find a plugin that defines an alias for "mv". Puzzled...
>
> - Lars
>
> [1] https://github.com/robbyrussell/oh-my-zsh
Oh. I must have made a mistake on my very first test run. I can reproduce
the failure with ZSH and my plugins... looks like it's a Mac OS problem
and no TravisCI only problem after all.
Sorry for the noise/confusion,
Lars
^ permalink raw reply
* Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Johannes Schindelin @ 2017-01-26 11:16 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, David Aguilar, Ramsay Jones, GIT Mailing-list
In-Reply-To: <20170125183542.pe5qolexqqx6jhsi@sigill.intra.peff.net>
Hi Peff,
On Wed, 25 Jan 2017, Jeff King wrote:
> On Wed, Jan 25, 2017 at 11:36:50AM +0100, Johannes Schindelin wrote:
>
> > > Gross, but at least it's self documenting. :)
> > >
> > > I guess a less horrible version of that is:
> > >
> > > static inline warning_blank_line(void)
> > > {
> > > warning("%s", "");
> > > }
> > >
> > > We'd potentially need a matching one for error(), but at last it avoids
> > > macro trickery.
> >
> > I fail to see how this function, or this definition, makes the code better
> > than simply calling `warning("%s", "");` and be done with it.
>
> The only advantage is that it is self-documenting, so somebody does not
> come through later and convert ("%s", "") back to ("").
We could switch the DEVELOPER option on by default, when gcc or clang is
used at least. Otherwise the DEVELOPER option (which I like very much)
would not be able to live up to its full potential.
Another thing we should consider: paying more attention to Continuous
Integration. At the moment, it happens quite frequently that `pu` builds
and passes the test suite fine on Linux, but neither on Windows nor on
MacOSX and it takes days to get the regressions fixed.
I vote for this patch:
> -- >8 --
> Subject: [PATCH] difftool: hack around -Wzero-length-format warning
>
> Building with "gcc -Wall" will complain that the format in:
>
> warning("")
>
> is empty. Which is true, but the warning is over-eager. We
> are calling the function for its side effect of printing
> "warning:", even with an empty string.
>
> Our DEVELOPER Makefile knob disables the warning, but not
> everybody uses it. Let's silence the warning in the code so
> that nobody reports it or tries to "fix" it.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/difftool.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index 42ad9e804..b5e85ab07 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -567,7 +567,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
> warning(_("both files modified: '%s' and '%s'."),
> wtdir.buf, rdir.buf);
> warning(_("working tree file has been left."));
> - warning("");
> + warning("%s", "");
> err = 1;
> } else if (unlink(wtdir.buf) ||
> copy_file(wtdir.buf, rdir.buf, st.st_mode))
> --
> 2.11.0.840.gd37c5973a
Ciao,
Dscho
^ permalink raw reply
* Re: sparse checkout - weird behavior
From: Paul Hammant @ 2017-01-26 11:20 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170126045936.wyleenuwunhrvbn2@sigill.intra.peff.net>
Well I feel a bit silly. Thanks for responding.
On Wed, Jan 25, 2017 at 11:59 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 25, 2017 at 10:21:19PM -0500, Paul Hammant wrote:
>
>> Related bug (maybe the same). Reproduction:
>>
>> $ git clone git@github.com:jekyll/jekyll.git --no-checkout
>> Cloning into 'jekyll'...
>> remote: Counting objects: 41331, done.
>> remote: Compressing objects: 100% (5/5), done.
>> remote: Total 41331 (delta 0), reused 0 (delta 0), pack-reused 41326
>> Receiving objects: 100% (41331/41331), 11.91 MiB | 9.15 MiB/s, done.
>> Resolving deltas: 100% (26530/26530), done.
>> $ cd jekyll
>> $ git config core.sparsecheckout true
>> $ echo 'CONDUCT.markdown' > .git/info/sparse-checkout
>> $ echo 'Gemfile' >> .git/info/sparse-checkout
>> $ echo 'Rakefile' >> .git/info/sparse-checkout
>> $ echo 'appveyor.yml' >> .git/info/sparse-checkout
>> $ git checkout --
>> Your branch is up-to-date with 'origin/master'.
>> $ ls
>> CONDUCT.markdown Gemfile Rakefile appveyor.yml lib
>>
>> I was not expecting to see 'lib' in the resulting file list
>
> Yep, I think this is the same problem. Inside lib, you get only
> "lib/theme_template/Gemfile", because it matches your unanchored
> pattern. Using "/Gemfile" in the sparse-checkout file fixes it.
>
> -Peff
^ permalink raw reply
* Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Johannes Schindelin @ 2017-01-26 11:37 UTC (permalink / raw)
To: Johannes Sixt
Cc: Jeff King, Junio C Hamano, David Aguilar, Ramsay Jones,
GIT Mailing-list
In-Reply-To: <546179e0-1d6e-86f7-00cf-e13218b76de1@kdbg.org>
Hi Hannes,
On Thu, 26 Jan 2017, Johannes Sixt wrote:
> Am 25.01.2017 um 23:01 schrieb Jeff King:
> > +#pragma GCC diagnostic ignored "-Wformat-zero-length"
>
> Last time I used #pragma GCC in a cross-platform project, it triggered
> an "unknown pragma" warning for MSVC.
It is starting to become a little funny how many ways we can discuss the
resolution of the GCC compiler warning.
And it starts to show: we try to solve the thing in so many ways, just to
avoid the obviously most-trivial patch to change warning(""); to
warning("%s", "") (the change to warning(" "); would change behavior, but
I would be fine with that, too).
I am not really interested in any of these complicated workarounds. If you
gentle people decide they are better in Git's source code, go ahead. I do
not have to like what you are doing, I just have to work with it.
> (It was the C++ compiler, I don't know if the C compiler would also
> warn.) It would have to be spelled like this:
>
> #pragma warning(disable: 4068) /* MSVC: unknown pragma */
> #pragma GCC diagnostic ignored "-Wformat-zero-length"
>
> Dscho mentioned that he's compiling with MSVC. It would do him a favor.
I am compiling with MSVC, and the idea is to tap into that large number of
Windows developers who Git traditionally has had a really bad time
attracting. From that perspective, I would say it would not only do me a
favor, but anybody who builds Git for Windows using Visual Studio.
But we also have to consider whether it would do anybody a "dis-favor".
#pragma statements are by definition highly dependent on the compiler. I
have no idea whether there are developers out there building Git with
C compilers other than GCC, clang or MSVC (as I did back in the days on
IRIX and HP/UX), but there is quite the potential for problems here [*1*].
To keep Git's source code truly portable, the #pragma would have to be
guarded by a GCC-specific #ifdef ... #endif.
Ciao,
Dscho
Footnote *1*: This is just another instance where a discussion on the Git
mailing list reminds me of
http://thedailywtf.com/articles/The_Complicator_0x27_s_Gloves, as it tries
to avoid an obvious solution by trying to come up with a different
solution that in turn requires additional solutions to additional problems
caused by the alternative solution.
^ permalink raw reply
* Re: [PATCH] Documentation: implement linkgit macro for Asciidoctor
From: Johannes Schindelin @ 2017-01-26 11:43 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Jeff King, Øyvind A. Holm
In-Reply-To: <20170126001344.445534-1-sandals@crustytoothpaste.net>
Hi Brian,
On Thu, 26 Jan 2017, brian m. carlson wrote:
> AsciiDoc uses a configuration file to implement macros like linkgit,
> while Asciidoctor uses Ruby extensions. Implement a Ruby extension that
> implements the linkgit macro for Asciidoctor in the same way that
> asciidoc.conf does for AsciiDoc. Adjust the Makefile to use it by
> default.
I like it.
Thank you,
Johannes
^ permalink raw reply
* Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
From: Johannes Schindelin @ 2017-01-26 12:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Segev Finer
In-Reply-To: <xmqqinp2939j.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 25 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Now, with the patch in question (without the follow-up, which I would
> > like to ask you to ignore, just like you did so far), Git would not
> > figure out that your script calls PuTTY eventually. The work-around?
> > Easy:
> >
> > DUMMY=/plink.exe /path/to/junio-is-a-superstar.sh
>
> Think about how you would explain that to an end-user in our document?
> You'll need to explain how exactly the auto-detection works, so that the
> user can "exploit" the loophole to do that. And what maintenance burden
> does it add when auto-detection is updated?
Fine, you do not like it. Saying so (instead of asking me questions) would
have been helpful.
> I think I know you well enough that you know well enough that it is too
> ugly to live, and I suspect that the above is a tongue-in-cheek "arguing
> for the sake of argument" and would not need a serious response, but
> just in case...
It was not tongue-in-cheek, I was being serious.
> Yes. Here is what comes on an obvious clean-up patch (which will be
> sent as a follow-up to this message).
I'd much rather prefer
https://github.com/git-for-windows/git/pull/1030 than your patch.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 2/2] fsck: lazily load types under --connectivity-only
From: Johannes Schindelin @ 2017-01-26 12:07 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170126041206.5qfv7r7czbwfqvaa@sigill.intra.peff.net>
Hi Peff,
On Wed, 25 Jan 2017, Jeff King wrote:
> builtin/fsck.c | 58 +++++++---------------------------------------------------
> fsck.c | 4 ++++
> 2 files changed, 11 insertions(+), 51 deletions(-)
Patch looks good to my eyes.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] Retire the `relink` command
From: Johannes Schindelin @ 2017-01-26 12:17 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Junio C Hamano
In-Reply-To: <20170125232051.GA25810@whir>
Hi Eric,
On Wed, 25 Jan 2017, Eric Wong wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> > Back in the olden days, when all objects were loose and rubber boots
> > were made out of wood, it made sense to try to share (immutable)
> > objects between repositories.
> >
> > Ever since the arrival of pack files, it is but an anachronism.
> >
> > Let's move the script to the contrib/examples/ directory and no longer
> > offer it.
>
> On the other hand, we have no idea if there are still people
> using it for whatever reason...
>
> I suggest we have a deprecation period where:
I would be fine with a deprecation phase, but that decision is solely on
Junio's shoulders.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] mingw: allow hooks to be .exe files
From: Johannes Schindelin @ 2017-01-26 12:21 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20170125211529.jq4halip4ndbff5k@sigill.intra.peff.net>
Hi Peff,
On Wed, 25 Jan 2017, Jeff King wrote:
> On Wed, Jan 25, 2017 at 05:58:42PM +0100, Johannes Schindelin wrote:
>
> > - if (access(path.buf, X_OK) < 0)
> > + if (access(path.buf, X_OK) < 0) {
> > +#ifdef STRIP_EXTENSION
> > + strbuf_addstr(&path, ".exe");
>
> I think STRIP_EXTENSION is a string. Should this line be:
>
> strbuf_addstr(&path, STRIP_EXTENSION);
Yep.
v2 coming,
Johannes
^ permalink raw reply
* [PATCH v2] mingw: allow hooks to be .exe files
From: Johannes Schindelin @ 2017-01-26 12:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <dc388b2df3baee83972f007cd77a57410553a411.1485362812.git.johannes.schindelin@gmx.de>
This change is necessary to allow the files in .git/hooks/ to optionally
have the file extension `.exe` on Windows, as the file names are
hardcoded otherwise.
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/exe-as-hook-v2
Fetch-It-Via: git fetch https://github.com/dscho/git exe-as-hook-v2
Interdiff vs v1:
diff --git a/run-command.c b/run-command.c
index 45229ef052..5227f78aea 100644
--- a/run-command.c
+++ b/run-command.c
@@ -873,7 +873,7 @@ const char *find_hook(const char *name)
strbuf_git_path(&path, "hooks/%s", name);
if (access(path.buf, X_OK) < 0) {
#ifdef STRIP_EXTENSION
- strbuf_addstr(&path, ".exe");
+ strbuf_addstr(&path, STRIP_EXTENSION);
if (access(path.buf, X_OK) >= 0)
return path.buf;
#endif
run-command.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/run-command.c b/run-command.c
index 73bfba7ef9..5227f78aea 100644
--- a/run-command.c
+++ b/run-command.c
@@ -871,8 +871,14 @@ const char *find_hook(const char *name)
strbuf_reset(&path);
strbuf_git_path(&path, "hooks/%s", name);
- if (access(path.buf, X_OK) < 0)
+ if (access(path.buf, X_OK) < 0) {
+#ifdef STRIP_EXTENSION
+ strbuf_addstr(&path, STRIP_EXTENSION);
+ if (access(path.buf, X_OK) >= 0)
+ return path.buf;
+#endif
return NULL;
+ }
return path.buf;
}
base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related
* Re: [PATCH 2/2] ref-filter: add function to parse atoms from a nul-terminated string
From: SZEDER Gábor @ 2017-01-26 13:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, SZEDER Gábor
In-Reply-To: <20161207160923.7028-3-szeder.dev@gmail.com>
On Wed, Dec 7, 2016 at 5:09 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> ref-filter's parse_ref_filter_atom() function parses an atom between
> the start and end pointers it gets as arguments. This is fine for two
> of its callers, which process '%(atom)' format specifiers and the end
> pointer comes directly from strchr() looking for the closing ')'.
> However, it's not quite so straightforward for its other two callers,
> which process sort specifiers given as plain nul-terminated strings.
> Especially not for ref_default_sorting(), which has the default
> hard-coded as a string literal, but can't use it directly, because a
> pointer to the end of that string literal is needed as well.
> The next patch will add yet another caller using a string literal.
>
> Add a helper function around parse_ref_filter_atom() to parse an atom
> up to the terminating nul, and call this helper in those two callers.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> ref-filter.c | 8 ++------
> ref-filter.h | 4 ++++
> 2 files changed, 6 insertions(+), 6 deletions(-)
Ping?
It looks like that this little two piece cleanup series fell on the floor.
> diff --git a/ref-filter.c b/ref-filter.c
> index dfadf577c..3c6fd4ba7 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1658,19 +1658,16 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
> /* If no sorting option is given, use refname to sort as default */
> struct ref_sorting *ref_default_sorting(void)
> {
> - static const char cstr_name[] = "refname";
> -
> struct ref_sorting *sorting = xcalloc(1, sizeof(*sorting));
>
> sorting->next = NULL;
> - sorting->atom = parse_ref_filter_atom(cstr_name, cstr_name + strlen(cstr_name));
> + sorting->atom = parse_ref_filter_atom_from_string("refname");
> return sorting;
> }
>
> void parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail)
> {
> struct ref_sorting *s;
> - int len;
>
> s = xcalloc(1, sizeof(*s));
> s->next = *sorting_tail;
> @@ -1683,8 +1680,7 @@ void parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail)
> if (skip_prefix(arg, "version:", &arg) ||
> skip_prefix(arg, "v:", &arg))
> s->version = 1;
> - len = strlen(arg);
> - s->atom = parse_ref_filter_atom(arg, arg+len);
> + s->atom = parse_ref_filter_atom_from_string(arg);
> }
>
> int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
> diff --git a/ref-filter.h b/ref-filter.h
> index 49466a17d..1250537cf 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -94,6 +94,10 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
> void ref_array_clear(struct ref_array *array);
> /* Parse format string and sort specifiers */
> int parse_ref_filter_atom(const char *atom, const char *ep);
> +static inline int parse_ref_filter_atom_from_string(const char *atom)
> +{
> + return parse_ref_filter_atom(atom, atom+strlen(atom));
> +}
> /* Used to verify if the given format is correct and to parse out the used atoms */
> int verify_ref_format(const char *format);
> /* Sort the given ref_array as per the ref_sorting provided */
> --
> 2.11.0.78.g5a2d011
>
^ permalink raw reply
* Re: SubmittingPatches: drop temporal reference for PGP signing
From: Cornelius Weig @ 2017-01-26 13:30 UTC (permalink / raw)
To: Stefan Beller, Philip Oakley
Cc: Johannes Sixt, bitte.keine.werbung.einwerfen, git@vger.kernel.org,
Junio C Hamano, thomas.braun, John Keeping
In-Reply-To: <CAGZ79kaRdtKD7DNJRWXsyg07GbTM4OsKUmHHcFczEMJA1YK2KA@mail.gmail.com>
>
> Yeah I agree. My patch was not the best shot by far.
>
How about something along these lines? Does the forward reference
break the main line of thought too severly?
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 08352de..c2b0cbe 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -216,12 +216,12 @@ that it will be postponed.
Exception: If your mailer is mangling patches then someone may ask
you to re-send them using MIME, that is OK.
-Do not PGP sign your patch, at least for now. Most likely, your
-maintainer or other people on the list would not have your PGP
-key and would not bother obtaining it anyway. Your patch is not
-judged by who you are; a good patch from an unknown origin has a
-far better chance of being accepted than a patch from a known,
-respected origin that is done poorly or does incorrect things.
+Do not PGP sign your patch, but do sign-off your work as explained in (5).
+Most likely, your maintainer or other people on the list would not have your
+PGP key and would not bother obtaining it anyway. Your patch is not judged by
+who you are; a good patch from an unknown origin has a far better chance of
+being accepted than a patch from a known, respected origin that is done poorly
+or does incorrect things.
If you really really really really want to do a PGP signed
patch, format it as "multipart/signed", not a text/plain message
@@ -246,7 +246,7 @@ patch.
*2* The mailing list: git@vger.kernel.org
-(5) Sign your work
+(5) Certify your work by signing off
To improve tracking of who did what, we've borrowed the
"sign-off" procedure from the Linux kernel project on patches
^ permalink raw reply related
* Re: [PATCH] mingw: allow hooks to be .exe files
From: Johannes Schindelin @ 2017-01-26 13:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq37g6akfx.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 25 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > This change is necessary to allow the files in .git/hooks/ to optionally
> > have the file extension `.exe` on Windows, as the file names are
> > hardcoded otherwise.
>
> Hmph. There is no longer ".com"?
No, no longer .com. You have to jump through hoops in this century to
build .com files.
> I briefly wondered if hooks/post-receive.{py,rb,...} would be good
> things to support, but I do not think we want to go there, primarily
> because we do not want to deal with "what happens when there are many?"
The answer is correct, the reasoning not. The reason why .exe is special:
it simply won't execute unless it has the .exe file extension. That is not
true for .py, .rb, etc
Ciao,
Johannes
^ permalink raw reply
* Re: [DEMO][PATCH v2 6/5] compat: add a qsort_s() implementation based on GNU's qsort_r(1)
From: Johannes Schindelin @ 2017-01-26 13:49 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Junio C Hamano, Git List
In-Reply-To: <20170125185153.obqwxiniyz2omxsi@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 425 bytes --]
Hi,
On Wed, 25 Jan 2017, Jeff King wrote:
> On Wed, Jan 25, 2017 at 07:43:01PM +0100, René Scharfe wrote:
>
> > If we find such cases then we'd better fix them for all platforms, e.g. by
> > importing timsort, no?
>
> Yes, as long as they are strict improvements.
I think in many cases, we may be better off by replacing the use of a
string-list for lookups by hashmaps for the same purpose.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] refs: add option core.logAllRefUpdates = always
From: Cornelius Weig @ 2017-01-26 14:06 UTC (permalink / raw)
To: Jeff King; +Cc: gitster, git, novalis, pclouds
In-Reply-To: <20170126033547.7bszipvkpi2jb4ad@sigill.intra.peff.net>
Hi Peff,
thanks for your thoughts.
> I tried to read this patch with fresh eyes. But given the history, you
> may take my review with a grain of salt. :)
Does it mean another reviewer is needed?
> I don't think my original had tests for this, but it might be worth
> adding a test for this last bit (i.e., that an update of ORIG_HEAD does
> not write a reflog when logallrefupdates is set to "always").
Good point. I blindly copied your commit message without thinking too
much about it.
> I guess the backtick fixups came from my original. It might be easier to
> see the change if they were pulled into their own patch, but it's
> probably not that big a deal.
If it's best practice to break out such changes, I'll revise it.
>> @@ -2835,8 +2835,8 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
>> {
>> int logfd, result, oflags = O_APPEND | O_WRONLY;
>>
>> - if (log_all_ref_updates < 0)
>> - log_all_ref_updates = !is_bare_repository();
>> + if (log_all_ref_updates == LOG_REFS_UNSET)
>> + log_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;
>
> This hunk is new, I think. The enum values are set in such a way that
> the original code would have continued to work, but I think using the
> symbolic names is an improvement.
Yes it's new.
> I assume you grepped for log_all_ref_updates to find this. I see only
> one spot that now doesn't use the symbolic names. In builtin/checkout.c,
> update_refs_for_switch() checks:
>
> if (opts->new_branch_log && !log_all_ref_updates)
>
> That looks buggy, as it would treat LOG_REFS_NORMAL and LOG_REFS_UNSET
> the same, and I do not see us resolving the UNSET case to a true/false
> value. But I don't think the bug is new in your patch; the default value
> was "-1" already.
>
> I doubt it can be triggered in practice, because either:
>
> - the config value is set in the config file, and we pick up that
> value, whether it's "true" or "false"
>
> - it's unset, in which case our default would be to enable reflogs in
> a non-bare repo. And since git-checkout would refuse to run in a
> bare repo, we must be non-bare, and thus enabling reflogs does the
> right thing.
That far I can follow.
> But it works quite by accident. I wonder if we should this
> "is_bare_repository" check into a function that can be called instead of
> accessing log_all_ref_updates() directly.
Are you saying that we should move the `!log_all_ref_updates` check into
its own function where we should also check `is_bare_repository`? I
don't see that this would win much, because as you said: checkouts in a
bare repo are forbidden anyway.
Other than that, I guess it should better read `log_all_ref_update !=
LOG_REFS_NONE` instead of `!log_all_ref_updates`.
>> +test_expect_success 'update-ref does not create reflog with --no-create-reflog if core.logAllRefUpdates=always' '
>
> This test title is _really_ long, and will wrap in the output on
> reasonable-sized terminals. Maybe '--no-create-reflog overrides
> core.logAllRefUpdates=always' would be shorter?
Yes, I agree.
>> +test_expect_success 'stdin does not create reflog when core.logAllRefUpdates=true' '
>
> I don't mind these extra stdin tests, but IMHO they are just redundant.
> The "--stdin --create-reflog" one makes sure the option is propagated
> down via the --stdin machinery. But we know the config option is handled
> at a low level anyway.
>
> I guess it depends on how black-box we want the testing to be. It just
> seems unlikely for a regression to be found here and not in the tests
> above.
Since these other stdin tests were around, I added this variant. But
you're right: this test breaks along with the other and doesn't add add
more safety. I'll remove it.
However, I realized that I have not written tests about ref updates in a
bare repository. Do you think it's worthwile?
Cheers,
Cornelius
^ permalink raw reply
* Re: [PATCH 2/5] revision.c: group ref selection options together
From: Jeff King @ 2017-01-26 14:19 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Jacob Keller
In-Reply-To: <CACsJy8CeUWD9ux3Q-VRhwR4-qbSD69CzH2mmztVH_-cx=31h8w@mail.gmail.com>
On Thu, Jan 26, 2017 at 04:18:06PM +0700, Duy Nguyen wrote:
> > Hmm. I see what you're trying to do here, and abstract the repeated
> > bits. But I'm not sure the line-count reflects a real simplification.
> > Everything ends up converted to an enum, and then that enum just expands
> > to similar C code.
>
> It's not simplification, but hopefully for better maintainability. This
>
> if (strcmp(arg, "--remotes")) {
> if (preceded_by_exclide())
> does_something();
> else if (preceded_by_decorate())
> does_another()
> } else if (strcmp(arg, "--branches")) {
> if (preceded_by_exclide())
> does_something();
> else if (preceded_by_decorate())
> does_another()
> }
>
> starts to look ugly especially when the third "preceded_by_" comes
> into picture. Putting all "does_something" in one group and
> "does_another" in another, I think, gives us a better view how ref
> selection is handled for a specific operation like --exclude or
> --decorate-ref.
I agree that would be ugly. But the current structure, which is:
if (strcmp(arg, "--remotes")) {
handle_refs(...);
cleanup();
} else if(...) {
handle_refs(...);
cleanup();
}
does not seem so bad, and pushes those conditionals into the
handle_refs() function, where they only need to be expressed once (I
didn't look, but I wonder if you could push the cleanup steps in there,
too, or if there is a caller who wants to handle() multiple times before
cleaning up).
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox