* 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
* Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude
From: Jeff King @ 2017-01-26 14:24 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Jacob Keller
In-Reply-To: <CACsJy8ATM_kc5SPY0dqprUefRy3vtpKW-4QEyJFK54jw0QgeJA@mail.gmail.com>
On Thu, Jan 26, 2017 at 04:28:17PM +0700, Duy Nguyen wrote:
> 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)?
I'd cross that bridge when we see what the option is. But my gut is that
rules would be:
- apply all non-conflicting relevant options. So:
--exclude=foo/* --decorate-refs --decorate-reflog --remotes
would presumably decorate both ref tips _and_ reflogs for all
remotes (except ones in refs/remotes/foo/*)
- for ones that are directly related and override each other,
use the usual last-one-wins rule. So:
--decorate-reflog --no-decorate-reflog --remotes
would countermand the original --decorate-reflog.
- for ones that really have complex interactions, notice and complain
in handle_refs().
That just seems to me like it follows our usual option parsing
procedure. The only difference here is that process and reset some
subset of the flags when we hit a special marker option ("--remotes" in
these examples) instead of doing it at the end.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
From: Jeff King @ 2017-01-26 14:26 UTC (permalink / raw)
To: Lars Schneider; +Cc: Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <8141FBB4-ACD0-42F5-9B5A-DA8DF1693972@gmail.com>
On Thu, Jan 26, 2017 at 10:48:30AM +0100, Lars Schneider wrote:
> 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.
Thanks for digging into it. If it's really /bin/mv that causes the
problem, then I doubly think the "mv -f" patch is the right fix.
-Peff
^ permalink raw reply
* Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Jeff King @ 2017-01-26 14:32 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, Johannes Schindelin, David Aguilar, Ramsay Jones,
GIT Mailing-list
In-Reply-To: <546179e0-1d6e-86f7-00cf-e13218b76de1@kdbg.org>
On Thu, Jan 26, 2017 at 07:39:55AM +0100, 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 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.
Bleh. The point of #pragma is to ignore ones you don't know about.
It would be easy to wrap it in an #ifdef for __GNUC__ (there is already
a similar pragma with similar wrapping in the code base).
Anyway. I do not want to make life harder for anyone. I think there are
several options floating around now, so I will let Junio decide which
one he wants to pick up.
-Peff
^ permalink raw reply
* Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Jeff King @ 2017-01-26 14:35 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Johannes Sixt, Junio C Hamano, David Aguilar, Ramsay Jones,
GIT Mailing-list
In-Reply-To: <alpine.DEB.2.20.1701261226220.3469@virtualbox>
On Thu, Jan 26, 2017 at 12:37:46PM +0100, Johannes Schindelin 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).
The point is that the trivial patch fixes _this_ case, but does not
prevent the discussion from happening again later. They are two separate
problems. I am OK not solving the latter one and relying on review (as
I've already said), but the solutions do not do the same thing.
-Peff
^ permalink raw reply
* Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Jeff King @ 2017-01-26 14:39 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, David Aguilar, Ramsay Jones, GIT Mailing-list
In-Reply-To: <alpine.DEB.2.20.1701261213060.3469@virtualbox>
On Thu, Jan 26, 2017 at 12:16:10PM +0100, Johannes Schindelin wrote:
> 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.
I'm not sure that is a good idea. The options include -Werror, which is
a good thing for developers to respect. But people using older versions
of compilers, or on systems with slightly different header files, may
see extraneous warnings. It's good to fix those warnings, but it is a
big inconvenience to regular users who just want to build and use git.
You could split the DEVELOPER options into two groups, though, and only
enable when (after verifying that it is indeed gcc/clang in use). But
now who is coming up with complicated fixes for the warning("") issue?
:)
-Peff
^ permalink raw reply
* Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
From: Johannes Schindelin @ 2017-01-26 14:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Segev Finer
In-Reply-To: <xmqqefzq936b.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 25 Jan 2017, Junio C Hamano wrote:
> Subject: [PATCH] connect: rename tortoiseplink and putty variables
>
> One of these two may have originally been named after "what exact
> SSH implementation do we have" so that we can tweak the command line
> options, but these days "putty=1" no longer means "We are using the
> plink SSH implementation that comes with PuTTY". It is set when we
> guess that either PuTTY plink or Tortoiseplink is in use.
>
> Rename them after what effect is desired. The current "putty"
> option is about using "-P <port>" when OpenSSH would use "-p <port>",
> so rename it to port_option whose value is either 'p' or 'P". The
> other one is about passing an extra command line option "-batch",
> so rename it needs_batch.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Apart from an overly-long line, this patch looks good. It made my life a
little harder, as I had to rebase Segev's ssh.variant (why this should be
a core.* variable is not clear to me) patch and it caused conflicts. I
resolved those conflicts and made both patches part of this patch series.
Will contribute v2 as soon as the test suite passes,
Johannes
^ permalink raw reply
* Re: [PATCH] refs: add option core.logAllRefUpdates = always
From: Jeff King @ 2017-01-26 14:46 UTC (permalink / raw)
To: Cornelius Weig; +Cc: gitster, git, novalis, pclouds
In-Reply-To: <4faf836a-40b6-da9a-877a-3b2ce7c863df@tngtech.com>
On Thu, Jan 26, 2017 at 03:06:40PM +0100, Cornelius Weig wrote:
> > 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.
Yes, I'm suggesting making something like the should_autocreate_reflog()
function public.
I agree it is working correctly now. It's just that it's rather subtle
that it treats LOG_REFS_UNSET implicitly as LOG_REFS_NONE.
It would also possibly break if more values are added to the enum
(depending on what those values are).
> However, I realized that I have not written tests about ref updates in a
> bare repository. Do you think it's worthwile?
There should already be a test for logAllRefUpdates=true in a bare
repository (if there isn't, we should probably add one). Testing the
"always" case individually does not add much over testing it in a
non-bare repository. IMHO.
-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 14:49 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, David Aguilar, Ramsay Jones, GIT Mailing-list
In-Reply-To: <20170126143906.j6j64v4cyatwvlik@sigill.intra.peff.net>
Hi Peff,
On Thu, 26 Jan 2017, Jeff King wrote:
> On Thu, Jan 26, 2017 at 12:16:10PM +0100, Johannes Schindelin wrote:
>
> > 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.
>
> I'm not sure that is a good idea. The options include -Werror, which is
> a good thing for developers to respect. But people using older versions
> of compilers, or on systems with slightly different header files, may
> see extraneous warnings. It's good to fix those warnings, but it is a
> big inconvenience to regular users who just want to build and use git.
Yeah, you cannot have the cake and eat it, too: on the one side, we want
Git contributors to see problems early (preferably before even submitting
the patch) and at the same time, we want users who compile their Git
themselves to have no trouble doing so.
> You could split the DEVELOPER options into two groups, though, and only
> enable when (after verifying that it is indeed gcc/clang in use). But
> now who is coming up with complicated fixes for the warning("") issue?
> :)
That is yet another instance of the complicator's glove; I would rather
avoid that.
To me, the obvious solution is to pay more attention to Continuous
Integration, in particular on fixing problems right after they are
reported.
Ciao,
Johannes
^ permalink raw reply
* [PATCH v2 0/3] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND
From: Johannes Schindelin @ 2017-01-26 14:51 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <2ff29a4d00e0e13d460122d8008e762361ca90aa.1483358673.git.johannes.schindelin@gmx.de>
We already handle PuTTY's plink and TortoiseGit's tortoiseplink in
GIT_SSH by automatically using the -P option to specify ports, and in
tortoiseplink's case by passing the --batch option.
For users who need to pass additional command-line options to plink,
this poses a problem: the only way to do that is to use GIT_SSH_COMMAND,
but Git does not handle that specifically, so those users have to
manually parse the command-line options passed via GIT_SSH_COMMAND and
replace -p (if present) by -P, and add --batch in the case of
tortoiseplink.
This is error-prone and a bad user experience.
To fix this, the changes proposed in this patch series introduce
handling this by splitting the GIT_SSH_COMMAND value and treating the
first parameter with the same grace as GIT_SSH. To counter any possible
misdetection, the user can also specify explicitly via GIT_SSH_VARIANT
or ssh.variant which SSH variant they are using.
This is v2 of the patch, now turned patch series. Relative to v1, it
integrates Junio's cleanup patch and Segev's follow-up Pull Request that
introduces the GIT_SSH_VARIANT and ssh.variant settings to override
Git's autodetection manually.
Junio C Hamano (1):
connect: rename tortoiseplink and putty variables
Segev Finer (2):
connect: handle putty/plink also in GIT_SSH_COMMAND
connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
Documentation/config.txt | 7 +++++
Documentation/git.txt | 7 +++++
connect.c | 66 +++++++++++++++++++++++++++++++++++-------------
t/t5601-clone.sh | 41 ++++++++++++++++++++++++++++++
4 files changed, 104 insertions(+), 17 deletions(-)
base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
Published-As: https://github.com/dscho/git/releases/tag/putty-w-args-v2
Fetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v2
Interdiff vs v1:
diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4cc02..f2c210f0a0 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1949,6 +1949,13 @@ Environment variable settings always override any matches. The URLs that are
matched against are those given directly to Git commands. This means any URLs
visited as a result of a redirection do not participate in matching.
+ssh.variant::
+ Override the autodetection of plink/tortoiseplink in the SSH
+ command that 'git fetch' and 'git push' use. It can be set to
+ either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
+ value will be treated as normal ssh. This is useful in case
+ that Git gets this wrong.
+
i18n.commitEncoding::
Character encoding the commit messages are stored in; Git itself
does not care per se, but this information is necessary e.g. when
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4f208fab92..c322558aa7 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1020,6 +1020,13 @@ Usually it is easier to configure any desired options through your
personal `.ssh/config` file. Please consult your ssh documentation
for further details.
+`GIT_SSH_VARIANT`::
+ If this environment variable is set, it overrides the autodetection
+ of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
+ push' use. It can be set to either `ssh`, `plink`, `putty` or
+ `tortoiseplink`. Any other value will be treated as normal ssh. This
+ is useful in case that Git gets this wrong.
+
`GIT_ASKPASS`::
If this environment variable is set, then Git commands which need to
acquire passwords or passphrases (e.g. for HTTP or IMAP authentication)
diff --git a/connect.c b/connect.c
index c81f77001b..7b4437578b 100644
--- a/connect.c
+++ b/connect.c
@@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
return NULL;
}
+static int handle_ssh_variant(int *port_option, int *needs_batch)
+{
+ const char *variant;
+
+ if (!(variant = getenv("GIT_SSH_VARIANT")) &&
+ git_config_get_string_const("ssh.variant", &variant))
+ return 0;
+
+ if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
+ *port_option = 'P';
+ else if (!strcmp(variant, "tortoiseplink")) {
+ *port_option = 'P';
+ *needs_batch = 1;
+ }
+
+ return 1;
+}
+
/*
* This returns a dummy child_process if the transport protocol does not
* need fork(2), or a struct child_process object if it does. Once done,
@@ -769,7 +787,8 @@ struct child_process *git_connect(int fd[2], const char *url,
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
const char *ssh;
- int putty = 0, tortoiseplink = 0;
+ int needs_batch = 0;
+ int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
char *ssh_argv0 = NULL;
@@ -816,28 +835,32 @@ struct child_process *git_connect(int fd[2], const char *url,
ssh_argv0 = xstrdup(ssh);
}
- if (ssh_argv0) {
+ if (!handle_ssh_variant(&port_option, &needs_batch) &&
+ ssh_argv0) {
const char *base = basename(ssh_argv0);
- tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
- !strcasecmp(base, "tortoiseplink.exe");
- putty = tortoiseplink ||
- !strcasecmp(base, "plink") ||
- !strcasecmp(base, "plink.exe");
-
- free(ssh_argv0);
+ if (!strcasecmp(base, "tortoiseplink") ||
+ !strcasecmp(base, "tortoiseplink.exe")) {
+ port_option = 'P';
+ needs_batch = 1;
+ } else if (!strcasecmp(base, "plink") ||
+ !strcasecmp(base, "plink.exe")) {
+ port_option = 'P';
+ }
}
+ free(ssh_argv0);
+
argv_array_push(&conn->args, ssh);
if (flags & CONNECT_IPV4)
argv_array_push(&conn->args, "-4");
else if (flags & CONNECT_IPV6)
argv_array_push(&conn->args, "-6");
- if (tortoiseplink)
+ if (needs_batch)
argv_array_push(&conn->args, "-batch");
if (port) {
- /* P is for PuTTY, p is for OpenSSH */
- argv_array_push(&conn->args, putty ? "-P" : "-p");
+ argv_array_pushf(&conn->args,
+ "-%c", port_option);
argv_array_push(&conn->args, port);
}
argv_array_push(&conn->args, ssh_host);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9335e10c2a..b52b8acf98 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -401,6 +401,32 @@ test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
expect_ssh "-v -P 123" myhost src
'
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+ GIT_SSH_VARIANT=ssh \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-1 &&
+ expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'ssh.variant overrides plink detection' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+ git -c ssh.variant=ssh \
+ clone "[myhost:123]:src" ssh-bracket-clone-variant-2 &&
+ expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
+ GIT_SSH_VARIANT=plink \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-3 &&
+ expect_ssh "-P 123" myhost src
+'
+
+test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '
+ GIT_SSH_VARIANT=tortoiseplink \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
+ expect_ssh "-batch -P 123" myhost src
+'
+
# Reset the GIT_SSH environment variable for clone tests.
setup_ssh_wrapper
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply
* [PATCH v2 1/3] connect: handle putty/plink also in GIT_SSH_COMMAND
From: Johannes Schindelin @ 2017-01-26 14:51 UTC (permalink / raw)
To: git; +Cc: Segev Finer, Junio C Hamano, Jeff King
In-Reply-To: <cover.1485442231.git.johannes.schindelin@gmx.de>
From: Segev Finer <segev208@gmail.com>
Git for Windows has special support for the popular SSH client PuTTY:
when using PuTTY's non-interactive version ("plink.exe"), we use the -P
option to specify the port rather than OpenSSH's -p option. TortoiseGit
ships with its own, forked version of plink.exe, that adds support for
the -batch option, and for good measure we special-case that, too.
However, this special-casing of PuTTY only covers the case where the
user overrides the SSH command via the environment variable GIT_SSH
(which allows specifying the name of the executable), not
GIT_SSH_COMMAND (which allows specifying a full command, including
additional command-line options).
When users want to pass any additional arguments to (Tortoise-)Plink,
such as setting a private key, they are required to either use a shell
script named plink or tortoiseplink or duplicate the logic that is
already in Git for passing the correct style of command line arguments,
which can be difficult, error prone and annoying to get right.
This patch simply reuses the existing logic and expands it to cover
GIT_SSH_COMMAND, too.
Note: it may look a little heavy-handed to duplicate the entire
command-line and then split it, only to extract the name of the
executable. However, this is not a performance-critical code path, and
the code is much more readable this way.
Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
connect.c | 23 ++++++++++++++++-------
t/t5601-clone.sh | 15 +++++++++++++++
2 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/connect.c b/connect.c
index 8cb93b0720..c81f77001b 100644
--- a/connect.c
+++ b/connect.c
@@ -772,6 +772,7 @@ struct child_process *git_connect(int fd[2], const char *url,
int putty = 0, tortoiseplink = 0;
char *ssh_host = hostandport;
const char *port = NULL;
+ char *ssh_argv0 = NULL;
transport_check_allowed("ssh");
get_host_and_port(&ssh_host, &port);
@@ -792,10 +793,15 @@ struct child_process *git_connect(int fd[2], const char *url,
}
ssh = get_ssh_command();
- if (!ssh) {
- const char *base;
- char *ssh_dup;
-
+ if (ssh) {
+ char *split_ssh = xstrdup(ssh);
+ const char **ssh_argv;
+
+ if (split_cmdline(split_ssh, &ssh_argv))
+ ssh_argv0 = xstrdup(ssh_argv[0]);
+ free(split_ssh);
+ free((void *)ssh_argv);
+ } else {
/*
* GIT_SSH is the no-shell version of
* GIT_SSH_COMMAND (and must remain so for
@@ -807,8 +813,11 @@ struct child_process *git_connect(int fd[2], const char *url,
if (!ssh)
ssh = "ssh";
- ssh_dup = xstrdup(ssh);
- base = basename(ssh_dup);
+ ssh_argv0 = xstrdup(ssh);
+ }
+
+ if (ssh_argv0) {
+ const char *base = basename(ssh_argv0);
tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
!strcasecmp(base, "tortoiseplink.exe");
@@ -816,7 +825,7 @@ struct child_process *git_connect(int fd[2], const char *url,
!strcasecmp(base, "plink") ||
!strcasecmp(base, "plink.exe");
- free(ssh_dup);
+ free(ssh_argv0);
}
argv_array_push(&conn->args, ssh);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 4241ea5b32..9335e10c2a 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -386,6 +386,21 @@ test_expect_success 'tortoiseplink is like putty, with extra arguments' '
expect_ssh "-batch -P 123" myhost src
'
+test_expect_success 'double quoted plink.exe in GIT_SSH_COMMAND' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+ GIT_SSH_COMMAND="\"$TRASH_DIRECTORY/plink.exe\" -v" \
+ git clone "[myhost:123]:src" ssh-bracket-clone-plink-3 &&
+ expect_ssh "-v -P 123" myhost src
+'
+
+SQ="'"
+test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+ GIT_SSH_COMMAND="$SQ$TRASH_DIRECTORY/plink.exe$SQ -v" \
+ git clone "[myhost:123]:src" ssh-bracket-clone-plink-4 &&
+ expect_ssh "-v -P 123" myhost src
+'
+
# Reset the GIT_SSH environment variable for clone tests.
setup_ssh_wrapper
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related
* [PATCH v2 2/3] connect: rename tortoiseplink and putty variables
From: Johannes Schindelin @ 2017-01-26 14:51 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Junio C Hamano, Jeff King
In-Reply-To: <cover.1485442231.git.johannes.schindelin@gmx.de>
From: Junio C Hamano <gitster@pobox.com>
One of these two may have originally been named after "what exact
SSH implementation do we have" so that we can tweak the command line
options, but these days "putty=1" no longer means "We are using the
plink SSH implementation that comes with PuTTY". It is set when we
guess that either PuTTY plink or Tortoiseplink is in use.
Rename them after what effect is desired. The current "putty"
option is about using "-P <port>" when OpenSSH would use "-p <port>",
so rename it to port_option whose value is either 'p' or 'P". The
other one is about passing an extra command line option "-batch",
so rename it needs_batch.
[jes: wrapped overly-long line]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
connect.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/connect.c b/connect.c
index c81f77001b..9f750eacb6 100644
--- a/connect.c
+++ b/connect.c
@@ -769,7 +769,8 @@ struct child_process *git_connect(int fd[2], const char *url,
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
const char *ssh;
- int putty = 0, tortoiseplink = 0;
+ int needs_batch = 0;
+ int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
char *ssh_argv0 = NULL;
@@ -819,12 +820,14 @@ struct child_process *git_connect(int fd[2], const char *url,
if (ssh_argv0) {
const char *base = basename(ssh_argv0);
- tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
- !strcasecmp(base, "tortoiseplink.exe");
- putty = tortoiseplink ||
- !strcasecmp(base, "plink") ||
- !strcasecmp(base, "plink.exe");
-
+ if (!strcasecmp(base, "tortoiseplink") ||
+ !strcasecmp(base, "tortoiseplink.exe")) {
+ port_option = 'P';
+ needs_batch = 1;
+ } else if (!strcasecmp(base, "plink") ||
+ !strcasecmp(base, "plink.exe")) {
+ port_option = 'P';
+ }
free(ssh_argv0);
}
@@ -833,11 +836,11 @@ struct child_process *git_connect(int fd[2], const char *url,
argv_array_push(&conn->args, "-4");
else if (flags & CONNECT_IPV6)
argv_array_push(&conn->args, "-6");
- if (tortoiseplink)
+ if (needs_batch)
argv_array_push(&conn->args, "-batch");
if (port) {
- /* P is for PuTTY, p is for OpenSSH */
- argv_array_push(&conn->args, putty ? "-P" : "-p");
+ argv_array_pushf(&conn->args,
+ "-%c", port_option);
argv_array_push(&conn->args, port);
}
argv_array_push(&conn->args, ssh_host);
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related
* [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
From: Johannes Schindelin @ 2017-01-26 14:52 UTC (permalink / raw)
To: git; +Cc: Segev Finer, Junio C Hamano, Jeff King
In-Reply-To: <cover.1485442231.git.johannes.schindelin@gmx.de>
From: Segev Finer <segev208@gmail.com>
This environment variable and configuration value allow to
override the autodetection of plink/tortoiseplink in case that
Git gets it wrong.
[jes: wrapped overly-long lines, changed get_ssh_variant() to
handle_ssh_variant() to accomodate the change from the
putty/tortoiseplink variables to port_option/needs_batch.]
Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/config.txt | 7 +++++++
Documentation/git.txt | 7 +++++++
connect.c | 24 ++++++++++++++++++++++--
t/t5601-clone.sh | 26 ++++++++++++++++++++++++++
4 files changed, 62 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4cc02..f2c210f0a0 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1949,6 +1949,13 @@ Environment variable settings always override any matches. The URLs that are
matched against are those given directly to Git commands. This means any URLs
visited as a result of a redirection do not participate in matching.
+ssh.variant::
+ Override the autodetection of plink/tortoiseplink in the SSH
+ command that 'git fetch' and 'git push' use. It can be set to
+ either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
+ value will be treated as normal ssh. This is useful in case
+ that Git gets this wrong.
+
i18n.commitEncoding::
Character encoding the commit messages are stored in; Git itself
does not care per se, but this information is necessary e.g. when
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4f208fab92..c322558aa7 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1020,6 +1020,13 @@ Usually it is easier to configure any desired options through your
personal `.ssh/config` file. Please consult your ssh documentation
for further details.
+`GIT_SSH_VARIANT`::
+ If this environment variable is set, it overrides the autodetection
+ of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
+ push' use. It can be set to either `ssh`, `plink`, `putty` or
+ `tortoiseplink`. Any other value will be treated as normal ssh. This
+ is useful in case that Git gets this wrong.
+
`GIT_ASKPASS`::
If this environment variable is set, then Git commands which need to
acquire passwords or passphrases (e.g. for HTTP or IMAP authentication)
diff --git a/connect.c b/connect.c
index 9f750eacb6..7b4437578b 100644
--- a/connect.c
+++ b/connect.c
@@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
return NULL;
}
+static int handle_ssh_variant(int *port_option, int *needs_batch)
+{
+ const char *variant;
+
+ if (!(variant = getenv("GIT_SSH_VARIANT")) &&
+ git_config_get_string_const("ssh.variant", &variant))
+ return 0;
+
+ if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
+ *port_option = 'P';
+ else if (!strcmp(variant, "tortoiseplink")) {
+ *port_option = 'P';
+ *needs_batch = 1;
+ }
+
+ return 1;
+}
+
/*
* This returns a dummy child_process if the transport protocol does not
* need fork(2), or a struct child_process object if it does. Once done,
@@ -817,7 +835,8 @@ struct child_process *git_connect(int fd[2], const char *url,
ssh_argv0 = xstrdup(ssh);
}
- if (ssh_argv0) {
+ if (!handle_ssh_variant(&port_option, &needs_batch) &&
+ ssh_argv0) {
const char *base = basename(ssh_argv0);
if (!strcasecmp(base, "tortoiseplink") ||
@@ -828,9 +847,10 @@ struct child_process *git_connect(int fd[2], const char *url,
!strcasecmp(base, "plink.exe")) {
port_option = 'P';
}
- free(ssh_argv0);
}
+ free(ssh_argv0);
+
argv_array_push(&conn->args, ssh);
if (flags & CONNECT_IPV4)
argv_array_push(&conn->args, "-4");
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9335e10c2a..b52b8acf98 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -401,6 +401,32 @@ test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
expect_ssh "-v -P 123" myhost src
'
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+ GIT_SSH_VARIANT=ssh \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-1 &&
+ expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'ssh.variant overrides plink detection' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+ git -c ssh.variant=ssh \
+ clone "[myhost:123]:src" ssh-bracket-clone-variant-2 &&
+ expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
+ GIT_SSH_VARIANT=plink \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-3 &&
+ expect_ssh "-P 123" myhost src
+'
+
+test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '
+ GIT_SSH_VARIANT=tortoiseplink \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
+ expect_ssh "-batch -P 123" myhost src
+'
+
# Reset the GIT_SSH environment variable for clone tests.
setup_ssh_wrapper
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related
* SoC Microprojects 2017
From: Pranit Bauva @ 2017-01-26 15:57 UTC (permalink / raw)
To: Git List
Hey everyone,
I helped in just re-organizing the micro project list for 2017. I have
removed the ones which I remember have been done and I have added one
new.
Please add any microproject if it comes to your mind or reply here so
I will add it.
Unfortunately, I can't send the patch here (SMTP blocked by institute
proxy) but I have included it as a link[1]. And here is the PR[2].
[1]: https://patch-diff.githubusercontent.com/raw/git/git.github.io/pull/219.patch
[2]: https://github.com/git/git.github.io/pull/219
Regards,
Pranit Bauva
^ permalink raw reply
* [PATCH v2 0/1] Let `git status` handle a not-yet-started `rebase -i` gracefully
From: Johannes Schindelin @ 2017-01-26 16:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Matthieu Moy, Guillaume Pages
In-Reply-To: <99f6de4be107044fdf01ee796f42e124ac147891.1453480067.git.johannes.schindelin@gmx.de>
When the `done` file is missing, we die()d. This is not necessary, we
can do much better than that.
Changes since v1:
- When `done` is missing, we still read `git-rebase-todo` and report the
next steps.
- We now report a missing git-rebase-todo.
- Added a test (thanks, Matthieu, for prodding me into working harder
;-)).
- As I changed so much, I took authorship of the patch.
Johannes Schindelin (1):
status: be prepared for not-yet-started interactive rebase
t/t7512-status-help.sh | 19 +++++++++++++++++++
wt-status.c | 14 ++++++++++----
2 files changed, 29 insertions(+), 4 deletions(-)
base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
Published-As: https://github.com/dscho/git/releases/tag/wt-status-v2
Fetch-It-Via: git fetch https://github.com/dscho/git wt-status-v2
Interdiff vs v1:
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index 5c3db656df..458608cc1e 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -944,4 +944,23 @@ EOF
test_i18ncmp expected actual
'
+test_expect_success 'status: handle not-yet-started rebase -i gracefully' '
+ ONTO=$(git rev-parse --short HEAD^) &&
+ COMMIT=$(git rev-parse --short HEAD) &&
+ EDITOR="git status --untracked-files=no >actual" git rebase -i HEAD^ &&
+ cat >expected <<EOF &&
+On branch several_commits
+No commands done.
+Next command to do (1 remaining command):
+ pick $COMMIT four_commit
+ (use "git rebase --edit-todo" to view and edit)
+You are currently editing a commit while rebasing branch '\''several_commits'\'' on '\''$ONTO'\''.
+ (use "git commit --amend" to amend the current commit)
+ (use "git rebase --continue" once you are satisfied with your changes)
+
+nothing to commit (use -u to show untracked files)
+EOF
+ test_i18ncmp expected actual
+'
+
test_done
diff --git a/wt-status.c b/wt-status.c
index 13afe66649..4dff0b3e21 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1169,12 +1169,12 @@ static void show_rebase_information(struct wt_status *s,
struct string_list have_done = STRING_LIST_INIT_DUP;
struct string_list yet_to_do = STRING_LIST_INIT_DUP;
- if ((read_rebase_todolist("rebase-merge/done", &have_done)) ||
- (read_rebase_todolist("rebase-merge/git-rebase-todo",
- &yet_to_do)))
+ read_rebase_todolist("rebase-merge/done", &have_done);
+ if (read_rebase_todolist("rebase-merge/git-rebase-todo",
+ &yet_to_do))
status_printf_ln(s, color,
- _("rebase-i not started yet."));
- else if (have_done.nr == 0)
+ _("git-rebase-todo is missing."));
+ if (have_done.nr == 0)
status_printf_ln(s, color, _("No commands done."));
else {
status_printf_ln(s, color,
@@ -1192,9 +1192,7 @@ static void show_rebase_information(struct wt_status *s,
_(" (see more in file %s)"), git_path("rebase-merge/done"));
}
- if (have_done.nr == 0)
- ; /* do nothing */
- else if (yet_to_do.nr == 0)
+ if (yet_to_do.nr == 0)
status_printf_ln(s, color,
_("No commands remaining."));
else {
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply
* [PATCH v2 1/1] status: be prepared for not-yet-started interactive rebase
From: Johannes Schindelin @ 2017-01-26 16:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Matthieu Moy, Guillaume Pages
In-Reply-To: <cover.1485446899.git.johannes.schindelin@gmx.de>
Some developers might want to call `git status` in a working
directory where they just started an interactive rebase, but the
edit script is still opened in the editor.
Let's show a meaningful message in such cases.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t7512-status-help.sh | 19 +++++++++++++++++++
wt-status.c | 14 ++++++++++----
2 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index 5c3db656df..458608cc1e 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -944,4 +944,23 @@ EOF
test_i18ncmp expected actual
'
+test_expect_success 'status: handle not-yet-started rebase -i gracefully' '
+ ONTO=$(git rev-parse --short HEAD^) &&
+ COMMIT=$(git rev-parse --short HEAD) &&
+ EDITOR="git status --untracked-files=no >actual" git rebase -i HEAD^ &&
+ cat >expected <<EOF &&
+On branch several_commits
+No commands done.
+Next command to do (1 remaining command):
+ pick $COMMIT four_commit
+ (use "git rebase --edit-todo" to view and edit)
+You are currently editing a commit while rebasing branch '\''several_commits'\'' on '\''$ONTO'\''.
+ (use "git commit --amend" to amend the current commit)
+ (use "git rebase --continue" once you are satisfied with your changes)
+
+nothing to commit (use -u to show untracked files)
+EOF
+ test_i18ncmp expected actual
+'
+
test_done
diff --git a/wt-status.c b/wt-status.c
index a715e71906..4dff0b3e21 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1135,14 +1135,17 @@ static void abbrev_sha1_in_line(struct strbuf *line)
strbuf_list_free(split);
}
-static void read_rebase_todolist(const char *fname, struct string_list *lines)
+static int read_rebase_todolist(const char *fname, struct string_list *lines)
{
struct strbuf line = STRBUF_INIT;
FILE *f = fopen(git_path("%s", fname), "r");
- if (!f)
+ if (!f) {
+ if (errno == ENOENT)
+ return -1;
die_errno("Could not open file %s for reading",
git_path("%s", fname));
+ }
while (!strbuf_getline_lf(&line, f)) {
if (line.len && line.buf[0] == comment_line_char)
continue;
@@ -1152,6 +1155,7 @@ static void read_rebase_todolist(const char *fname, struct string_list *lines)
abbrev_sha1_in_line(&line);
string_list_append(lines, line.buf);
}
+ return 0;
}
static void show_rebase_information(struct wt_status *s,
@@ -1166,8 +1170,10 @@ static void show_rebase_information(struct wt_status *s,
struct string_list yet_to_do = STRING_LIST_INIT_DUP;
read_rebase_todolist("rebase-merge/done", &have_done);
- read_rebase_todolist("rebase-merge/git-rebase-todo", &yet_to_do);
-
+ if (read_rebase_todolist("rebase-merge/git-rebase-todo",
+ &yet_to_do))
+ status_printf_ln(s, color,
+ _("git-rebase-todo is missing."));
if (have_done.nr == 0)
status_printf_ln(s, color, _("No commands done."));
else {
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related
* PATCH 1/2] abspath: add absolute_pathdup()
From: René Scharfe @ 2017-01-26 17:47 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
Add a function that returns a buffer containing the absolute path of its
argument and a semantic patch for its intended use. It avoids an extra
string copy to a static buffer.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
abspath.c | 7 +++++++
cache.h | 1 +
contrib/coccinelle/xstrdup_or_null.cocci | 6 ++++++
3 files changed, 14 insertions(+)
diff --git a/abspath.c b/abspath.c
index fce40fddcc..2f0c26e0e2 100644
--- a/abspath.c
+++ b/abspath.c
@@ -239,6 +239,13 @@ const char *absolute_path(const char *path)
return sb.buf;
}
+char *absolute_pathdup(const char *path)
+{
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_add_absolute_path(&sb, path);
+ return strbuf_detach(&sb, NULL);
+}
+
/*
* Unlike prefix_path, this should be used if the named file does
* not have to interact with index entry; i.e. name of a random file
diff --git a/cache.h b/cache.h
index 00a029af36..d7b7a8cd7a 100644
--- a/cache.h
+++ b/cache.h
@@ -1069,6 +1069,7 @@ const char *real_path(const char *path);
const char *real_path_if_valid(const char *path);
char *real_pathdup(const char *path);
const char *absolute_path(const char *path);
+char *absolute_pathdup(const char *path);
const char *remove_leading_path(const char *in, const char *prefix);
const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);
int normalize_path_copy_len(char *dst, const char *src, int *prefix_len);
diff --git a/contrib/coccinelle/xstrdup_or_null.cocci b/contrib/coccinelle/xstrdup_or_null.cocci
index 3fceef132b..8e05d1ca4b 100644
--- a/contrib/coccinelle/xstrdup_or_null.cocci
+++ b/contrib/coccinelle/xstrdup_or_null.cocci
@@ -5,3 +5,9 @@ expression V;
- if (E)
- V = xstrdup(E);
+ V = xstrdup_or_null(E);
+
+@@
+expression E;
+@@
+- xstrdup(absolute_path(E))
++ absolute_pathdup(E)
--
2.11.0
^ permalink raw reply related
* [PATCH 2/2] use absolute_pathdup()
From: René Scharfe @ 2017-01-26 17:54 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
In-Reply-To: <d94d742d-1247-ac35-c081-7db1f2178d34@web.de>
Apply the symantic patch for converting callers that duplicate the
result of absolute_path() to call absolute_pathdup() instead, which
avoids an extra string copy to a static buffer.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
builtin/clone.c | 4 ++--
builtin/submodule--helper.c | 2 +-
worktree.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 5ef81927a6..3f63edbbf9 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -170,7 +170,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
strbuf_addstr(&path, repo);
raw = get_repo_path_1(&path, is_bundle);
- canon = raw ? xstrdup(absolute_path(raw)) : NULL;
+ canon = raw ? absolute_pathdup(raw) : NULL;
strbuf_release(&path);
return canon;
}
@@ -894,7 +894,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
path = get_repo_path(repo_name, &is_bundle);
if (path)
- repo = xstrdup(absolute_path(repo_name));
+ repo = absolute_pathdup(repo_name);
else if (!strchr(repo_name, ':'))
die(_("repository '%s' does not exist"), repo_name);
else
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 74614a951e..899dc334e3 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -626,7 +626,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
module_clone_options);
strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
- sm_gitdir = xstrdup(absolute_path(sb.buf));
+ sm_gitdir = absolute_pathdup(sb.buf);
strbuf_reset(&sb);
if (!is_absolute_path(path)) {
diff --git a/worktree.c b/worktree.c
index 53b4771c04..d633761575 100644
--- a/worktree.c
+++ b/worktree.c
@@ -145,7 +145,7 @@ static struct worktree *get_linked_worktree(const char *id)
static void mark_current_worktree(struct worktree **worktrees)
{
- char *git_dir = xstrdup(absolute_path(get_git_dir()));
+ char *git_dir = absolute_pathdup(get_git_dir());
int i;
for (i = 0; worktrees[i]; i++) {
--
2.11.0
^ permalink raw reply related
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