* [PATCH 2/6] worktree.c: add update_worktree_location()
From: Nguyễn Thái Ngọc Duy @ 2017-01-08 9:39 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20170108094003.637-1-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
worktree.c | 21 +++++++++++++++++++++
worktree.h | 6 ++++++
2 files changed, 27 insertions(+)
diff --git a/worktree.c b/worktree.c
index 929072a..7684951 100644
--- a/worktree.c
+++ b/worktree.c
@@ -354,6 +354,27 @@ int validate_worktree(const struct worktree *wt, int quiet)
return 0;
}
+int update_worktree_location(struct worktree *wt, const char *path_)
+{
+ struct strbuf path = STRBUF_INIT;
+ int ret = 0;
+
+ if (is_main_worktree(wt))
+ return 0;
+
+ strbuf_add_absolute_path(&path, path_);
+ if (fspathcmp(wt->path, path.buf)) {
+ write_file(git_common_path("worktrees/%s/gitdir",
+ wt->id),
+ "%s/.git", real_path(path.buf));
+ free(wt->path);
+ wt->path = strbuf_detach(&path, NULL);
+ ret = 0;
+ }
+ strbuf_release(&path);
+ return ret;
+}
+
int is_worktree_being_rebased(const struct worktree *wt,
const char *target)
{
diff --git a/worktree.h b/worktree.h
index 4433db2..1ee03f4 100644
--- a/worktree.h
+++ b/worktree.h
@@ -57,6 +57,12 @@ extern const char *is_worktree_locked(struct worktree *wt);
*/
extern int validate_worktree(const struct worktree *wt, int quiet);
+/*
+ * Update worktrees/xxx/gitdir with the new path.
+ */
+extern int update_worktree_location(struct worktree *wt,
+ const char *path_);
+
/*
* Free up the memory for worktree(s)
*/
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 1/6] worktree.c: add validate_worktree()
From: Nguyễn Thái Ngọc Duy @ 2017-01-08 9:39 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20170108094003.637-1-pclouds@gmail.com>
This function is later used by "worktree move" and "worktree remove"
to ensure that we have a good connection between the repository and
the worktree. For example, if a worktree is moved manually, the
worktree location recorded in $GIT_DIR/worktrees/.../gitdir is
incorrect and we should not move that one.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
worktree.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
worktree.h | 5 +++++
2 files changed, 68 insertions(+)
diff --git a/worktree.c b/worktree.c
index eb61212..929072a 100644
--- a/worktree.c
+++ b/worktree.c
@@ -291,6 +291,69 @@ const char *is_worktree_locked(struct worktree *wt)
return wt->lock_reason;
}
+static int report(int quiet, const char *fmt, ...)
+{
+ va_list params;
+
+ if (quiet)
+ return -1;
+
+ va_start(params, fmt);
+ vfprintf(stderr, fmt, params);
+ fputc('\n', stderr);
+ va_end(params);
+ return -1;
+}
+
+int validate_worktree(const struct worktree *wt, int quiet)
+{
+ struct strbuf sb = STRBUF_INIT;
+ const char *path;
+ int err;
+
+ if (is_main_worktree(wt)) {
+ /*
+ * Main worktree using .git file to point to the
+ * repository would make it impossible to know where
+ * the actual worktree is if this function is executed
+ * from another worktree. No .git file support for now.
+ */
+ strbuf_addf(&sb, "%s/.git", wt->path);
+ if (!is_directory(sb.buf)) {
+ strbuf_release(&sb);
+ return report(quiet, _("'%s/.git' at main worktree is not the repository directory"),
+ wt->path);
+ }
+ return 0;
+ }
+
+ /*
+ * Make sure "gitdir" file points to a real .git file and that
+ * file points back here.
+ */
+ if (!is_absolute_path(wt->path))
+ return report(quiet, _("'%s' file does not contain absolute path to the worktree location"),
+ git_common_path("worktrees/%s/gitdir", wt->id));
+
+ strbuf_addf(&sb, "%s/.git", wt->path);
+ if (!file_exists(sb.buf)) {
+ strbuf_release(&sb);
+ return report(quiet, _("'%s/.git' does not exist"), wt->path);
+ }
+
+ path = read_gitfile_gently(sb.buf, &err);
+ strbuf_release(&sb);
+ if (!path)
+ return report(quiet, _("'%s/.git' is not a .git file, error code %d"),
+ wt->path, err);
+
+ if (fspathcmp(path, real_path(git_common_path("worktrees/%s", wt->id))))
+ return report(quiet, _("'%s' does not point back to"),
+ wt->path, git_common_path("worktrees/%s", wt->id));
+
+ return 0;
+}
+
int is_worktree_being_rebased(const struct worktree *wt,
const char *target)
{
diff --git a/worktree.h b/worktree.h
index d59ce1f..4433db2 100644
--- a/worktree.h
+++ b/worktree.h
@@ -52,6 +52,11 @@ extern int is_main_worktree(const struct worktree *wt);
*/
extern const char *is_worktree_locked(struct worktree *wt);
+/*
+ * Return zero if the worktree is in good condition.
+ */
+extern int validate_worktree(const struct worktree *wt, int quiet);
+
/*
* Free up the memory for worktree(s)
*/
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 0/6] git worktree move/remove
From: Nguyễn Thái Ngọc Duy @ 2017-01-08 9:39 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
This version is the same as nd/worktree-move but with the recursive
directory copy code removed. We rely on rename() to move directories.
Nguyễn Thái Ngọc Duy (6):
worktree.c: add validate_worktree()
worktree.c: add update_worktree_location()
worktree move: new command
worktree move: accept destination as directory
worktree move: refuse to move worktrees with submodules
worktree remove: new command
Documentation/git-worktree.txt | 28 ++++--
builtin/worktree.c | 162 +++++++++++++++++++++++++++++++++
contrib/completion/git-completion.bash | 5 +-
t/t2028-worktree-move.sh | 56 ++++++++++++
worktree.c | 84 +++++++++++++++++
worktree.h | 11 +++
6 files changed, 335 insertions(+), 11 deletions(-)
--
2.8.2.524.g6ff3d78
^ permalink raw reply
* [PATCH] Makefile: put LIBS after LDFLAGS for imap-send
From: Steven Penny @ 2017-01-08 6:12 UTC (permalink / raw)
To: git; +Cc: Steven Penny
This matches up with the targets git-%, git-http-fetch, git-http-push and
git-remote-testsvn. It must be done this way on Windows else lcrypto cannot find
lgdi32 and lws2_32
Signed-off-by: Steven Penny <svnpenn@gmail.com>
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index d861bd9..f0f65ea 100644
--- a/Makefile
+++ b/Makefile
@@ -2046,7 +2046,7 @@ git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
- $(LIBS) $(IMAP_SEND_LDFLAGS)
+ $(IMAP_SEND_LDFLAGS) $(LIBS)
git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
--
2.8.3
^ permalink raw reply related
* Re: "git fsck" not detecting garbage at the end of blob object files...
From: Jeff King @ 2017-01-08 5:26 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: John Szakmeister, git
In-Reply-To: <1483825623.31837.9.camel@kaarsemaker.net>
On Sat, Jan 07, 2017 at 10:47:03PM +0100, Dennis Kaarsemaker wrote:
> On Sat, 2017-01-07 at 07:50 -0500, John Szakmeister wrote:
> > I was perusing StackOverflow this morning and ran across this
> > question: http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/
> >
> > It was a simple question about why "checking objects" was not
> > appearing, but in it was another issue. The user purposefully
> > corrupted a blob object file to see if `git fsck` would catch it by
> > tacking extra data on at the end. `git fsck` happily said everything
> > was okay, but when I played with things locally I found out that `git
> > gc` does not like that extra garbage. I'm not sure what the trade-off
> > needs to be here, but my expectation is that if `git fsck` says
> > everything is okay, then all operations using that object (file)
> > should work too.
> >
> > Is that unreasonable? What would be the impact of fixing this issue?
>
> If you do this with a commit object or tree object, fsck does complain.
> I think it's sensible to do so for blob objects as well.
The existing extra-garbage check is in unpack_sha1_rest(), which is
called as part of read_sha1_file(). And that's what we hit for commits
and trees. However, we check the sha1 of blobs using the streaming
interface (in case they're large). I think you'd want to put a similar
check into read_istream_loose(). But note if you are grepping for it, it
is hidden behind a macro; look for read_method_decl(loose).
I'm actually not sure if this should be downgrade to a warning. It's
true that it's a form of corruption, but it doesn't actually prohibit us
from getting the data we need to complete the operation. Arguably fsck
should be more picky, but it is just relying on the same parse_object()
code path that the rest of git uses.
I doubt anybody cares too much either way, though. It's not like this is
a common thing.
I did notice another interesting case when looking at this. Fsck ends up
in fsck_loose(), which has the sha1 and path of the loose object. It
passes the sha1 to fsck_sha1(), and ignores the path entirely!
So if you have a duplicate copy of the object in a pack, we'd actually
find and check the duplicate. This can happen, e.g., if you had a loose
object and fetched a thin-pack which made a copy of the loose object to
complete the pack).
Probably fsck_loose() should be more picky about making sure we are
reading the data from the loose version we found.
-Peff
^ permalink raw reply
* Re: [PATCH 0/3] fixing some random blame corner cases
From: Junio C Hamano @ 2017-01-08 3:39 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170106041541.rjzzofal5hscv6yi@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> [1/3]: blame: fix alignment with --abbrev=40
> [2/3]: blame: handle --no-abbrev
> [3/3]: blame: output porcelain "previous" header for each file
Thanks. 1 & 2 obviously look correct. I'd need to look at 3 when I
am not exhausted, even though I expect it to be also fine from a
cursory read.
^ permalink raw reply
* Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`
From: Jeff King @ 2017-01-08 3:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git, 마누엘
In-Reply-To: <xmqqbmvi34ul.fsf@gitster.mtv.corp.google.com>
On Sat, Jan 07, 2017 at 02:03:30PM -0800, Junio C Hamano wrote:
> Is that a longer way to say that the claim "... is designed as a
> book" is false?
>
> > So I dunno. I really do think "article" is conceptually the most
> > appropriate style, but I agree that there are some book-like things
> > (like appendices).
>
> ... Yeah, I should have read forward first before starting to insert
> my comments.
To be honest, I'm not sure whether "book" versus "article" was really
considered in the original writing. I think we can call it whichever
produces the output we find most pleasing. I was mostly just pointing at
there are some tradeoffs in the end result in flipping the type.
-Peff
^ permalink raw reply
* Re: [PATCH] rebase--interactive: count squash commits above 10 correctly
From: Junio C Hamano @ 2017-01-08 3:26 UTC (permalink / raw)
To: Jeff King; +Cc: Brandon Tolsch, Johannes Schindelin, Vasco Almeida, git
In-Reply-To: <20170107082318.jj3izthx2ylscrns@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> git-rebase--interactive.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index b0a6f2b7ba..4734094a3f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -425,7 +425,7 @@ update_squash_messages () {
> if test -f "$squash_msg"; then
> mv "$squash_msg" "$squash_msg".bak || exit
> count=$(($(sed -n \
> - -e "1s/^$comment_char.*\([0-9][0-9]*\).*/\1/p" \
> + -e "1s/^$comment_char[^0-9]*\([0-9][0-9]*\).*/\1/p" \
I would have written it to match ".*[^1-9]\([1-9][0-9]*\)", though.
Even if a foreign language expresses all its words as digits (or has
a digit as the last letter of the last word before the number), a
translation into it must have a non-digit before the number to
disambiguate---but I guess I am being ultra-pedantic, and I think
what you wrote would be sufficient in practice.
As you guys discussed, this code will hopefully disappear in a cycle
or two anyway ;-) Let's queue this as-is.
Thanks.
> -e "q" < "$squash_msg".bak)+1))
> {
> printf '%s\n' "$comment_char $(eval_ngettext \
^ permalink raw reply
* Re: [PATCH v5 0/5] road to reentrant real_path
From: Junio C Hamano @ 2017-01-08 3:09 UTC (permalink / raw)
To: Brandon Williams
Cc: git, sbeller, peff, jacob.keller, ramsay, tboegi, j6t, pclouds,
larsxschneider
In-Reply-To: <20170104220124.145808-1-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> changes in v5:
> * set errno to ELOOP when MAXSYMLINKS is exceded.
> * revert to use MAXSYMLINKS instead of MAXDEPTH.
> * If the OS hasn't defined MAXSYMLINKS, use a fallback value of 32.
>
> Brandon Williams (4):
> real_path: resolve symlinks by hand
> real_path: convert real_path_internal to strbuf_realpath
> real_path: create real_pathdup
> real_path: have callers use real_pathdup and strbuf_realpath
>
> Johannes Sixt (1):
> real_path: canonicalize directory separators in root parts
>
How does this relate to the 5-patch real_path: series that has been
on 'next' since last year?
^ permalink raw reply
* Re: [PATCH] diff: add interhunk context config option
From: Junio C Hamano @ 2017-01-08 3:02 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Vegard Nossum, Git List, René Scharfe
In-Reply-To: <CAFZEwPMLk5KTeLR9zygU6ZH5zN7TLnQzmJVpyCNig8FrYcOazw@mail.gmail.com>
Pranit Bauva <pranit.bauva@gmail.com> writes:
>> diff --git a/diff.c b/diff.c
>> index 84dba60c4..40b4e6afe 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -33,6 +33,7 @@ static int diff_rename_limit_default = 400;
>> static int diff_suppress_blank_empty;
>> static int diff_use_color_default = -1;
>> static int diff_context_default = 3;
>> +static int diff_interhunk_context_default = 0;
Do not explicitly initialize BSS variables to 0 (or NULL).
>> static const char *diff_word_regex_cfg;
>> static const char *external_diff_cmd_cfg;
>> static const char *diff_order_file_cfg;
>> @@ -248,6 +249,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
>> return -1;
>> return 0;
>> }
>> + if (!strcmp(var, "diff.interhunkcontext")) {
>> + diff_interhunk_context_default = git_config_int(var, value);
>> + if (diff_interhunk_context_default < 0)
>> + return -1;
>> + return 0;
>> + }
>> if (!strcmp(var, "diff.renames")) {
>> diff_detect_rename_default = git_config_rename(var, value);
>> return 0;
>> @@ -3371,6 +3378,7 @@ void diff_setup(struct diff_options *options)
>> options->rename_limit = -1;
>> options->dirstat_permille = diff_dirstat_permille_default;
>> options->context = diff_context_default;
>> + options->interhunkcontext = diff_interhunk_context_default;
Will this receive -1 if diff.interhunkcontext configuration variable
is misconfigured and the *_default variable receives the error return
value from git_config_int()?
>> options->ws_error_highlight = ws_error_highlight_default;
>> DIFF_OPT_SET(options, RENAME_EMPTY);
>
> On a first look, it seems that we can overwrite the default config
> values by using a different command line argument which is good.
>
> Also, tests are missing. It seems that t/t4032 might be a good place
> to add those tests.
>
> Rest all is quite good! :)
>
> Regards,
> Pranit Bauva
^ permalink raw reply
* Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
From: Junio C Hamano @ 2017-01-08 2:33 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer
In-Reply-To: <2ff29a4d00e0e13d460122d8008e762361ca90aa.1483358673.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> + 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");
I suspect that this will break when GIT_SSH_COMMAND, which is meant
to be processed by the shell, hence the user can write anything,
begins with a one-shot environment variable assignment, e.g.
[core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink
One possible unintended side effect of this patch is when VAL1 ends
with /plink (or /tortoiseplink) and the command is not either of
these, in which case the "tortoiseplink" and "putty" variables will
tweak the command line for an SSH implementation that does not want
such a tweak to be made. There may be other unintended fallouts.
Sorry, no concrete suggestion to get this to work comes to my mind
offhand.
Perhaps give an explicit way to force "tortoiseplink" and "putty"
variables without looking at and guessing from the pathname, so that
the solution does not have to split and peek the command line?
connect.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/connect.c b/connect.c
index 8cb93b0720..1768122456 100644
--- a/connect.c
+++ b/connect.c
@@ -691,6 +691,23 @@ static const char *get_ssh_command(void)
return NULL;
}
+static void get_ssh_variant(int *tortoiseplink, int *putty)
+{
+ const char *variant;
+ if (!git_config_get_string_const("ssh.variant", &variant))
+ return;
+ if (!strcmp(variant, "tortoiseplink")) {
+ *tortoiseplink = 1;
+ *putty = 1;
+ } else if (!strcmp(variant, "putty")) {
+ *tortoiseplink = 0;
+ *putty = 1;
+ } else {
+ *tortoiseplink = 0;
+ *putty = 0;
+ }
+}
+
/*
* 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,
@@ -824,6 +841,9 @@ 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");
+
+ get_ssh_variant(&tortoiseplink, &putty);
+
if (tortoiseplink)
argv_array_push(&conn->args, "-batch");
if (port) {
^ permalink raw reply related
* Re: [PATCH v2 05/34] sequencer (rebase -i): learn about the 'verbose' mode
From: Junio C Hamano @ 2017-01-08 1:57 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <xmqqk2a635it.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> ...
>>
>> I guess my only defence is that I tried to be a little lazy.
>
> I actually was alluding to going the other way around, spawning
> "diff-tree -p" in the other codepath like this one does.
Pre-emptively, because I expect it will take a while for me to clear
the backlog to get to your reroll already on the mailing list, I do
not mean to say that you must rewrite the other one to spawn to
match this one. I meant that I wouldn't have minded if the series
were done that way, as this kind of being "little lazy" would reduce
the chance of regression by reducing the number of exchanged parts
and it is not necessarily a bad thing.
^ permalink raw reply
* Re: [PATCH 0/3] fix ^C killing pager when running alias
From: Jacob Keller @ 2017-01-07 23:27 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Sixt, Trygve Aaberge,
Nguyễn Thái Ngọc Duy, Git mailing list
In-Reply-To: <CA+P7+xqEqKWmQGgAyrmdzOZgO0CXFOGfcp=0otJ_nQPS13wFWg@mail.gmail.com>
On Sat, Jan 7, 2017 at 3:26 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Fri, Jan 6, 2017 at 5:14 PM, Jeff King <peff@peff.net> wrote:
>> On Fri, Jan 06, 2017 at 06:20:42PM -0500, Jeff King wrote:
>>
>>> > In general, I think it is wrong to wait for child processes when a signal
>>> > was received. After all, it is the purpose of a (deadly) signal to have the
>>> > process go away. There may be programs that know it better, like less, but
>>> > git should not attempt to know better in general.
>>> >
>>> > We do apply some special behavior for certain cases like we do for the
>>> > pager. And now the case with aliases is another special situation. The
>>> > parent git process only delegates to the child, and as such it is reasonable
>>> > that it binds its life time to the first child, which executes the expanded
>>> > alias.
>>>
>>> Yeah, I think I agree. That binding is something you want in many cases,
>>> but not necessarily all. The original purpose of clean_on_exit was to
>>> create a binding like that, but of course it can be (and with the
>>> smudge-filter stuff, arguably has been) used for other cases, too.
>>>
>>> I'll work up a patch that makes it a separate option, which should be
>>> pretty easy.
>>
>> Yeah, this did turn out to be really easy. I spent most of the time
>> trying to explain the issue in the commit message in a sane way.
>> Hopefully it didn't end up _too_ long. :)
>>
>> The interesting bit is in the third one. The first is a necessary
>> preparatory step, and the second is a cleanup I noticed in the
>> neighborhood.
>>
>> [1/3]: execv_dashed_external: use child_process struct
>> [2/3]: execv_dashed_external: stop exiting with negative code
>> [3/3]: execv_dashed_external: wait for child on signal death
>>
>> git.c | 36 +++++++++++++++---------------------
>> run-command.c | 19 +++++++++++++++++++
>> run-command.h | 1 +
>> 3 files changed, 35 insertions(+), 21 deletions(-)
>>
>> -Peff
>
> I don't see the rest of the patches on the list..?
>
> Thanks,
> Jake
They showed up on public inbox so I assume it must be some spam filter
on my end..
Hmm,
Jake
^ permalink raw reply
* Re: [PATCH 0/3] fix ^C killing pager when running alias
From: Jacob Keller @ 2017-01-07 23:26 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Sixt, Trygve Aaberge,
Nguyễn Thái Ngọc Duy, Git mailing list
In-Reply-To: <20170107011445.3e4fv6vdtimrwhgv@sigill.intra.peff.net>
On Fri, Jan 6, 2017 at 5:14 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Jan 06, 2017 at 06:20:42PM -0500, Jeff King wrote:
>
>> > In general, I think it is wrong to wait for child processes when a signal
>> > was received. After all, it is the purpose of a (deadly) signal to have the
>> > process go away. There may be programs that know it better, like less, but
>> > git should not attempt to know better in general.
>> >
>> > We do apply some special behavior for certain cases like we do for the
>> > pager. And now the case with aliases is another special situation. The
>> > parent git process only delegates to the child, and as such it is reasonable
>> > that it binds its life time to the first child, which executes the expanded
>> > alias.
>>
>> Yeah, I think I agree. That binding is something you want in many cases,
>> but not necessarily all. The original purpose of clean_on_exit was to
>> create a binding like that, but of course it can be (and with the
>> smudge-filter stuff, arguably has been) used for other cases, too.
>>
>> I'll work up a patch that makes it a separate option, which should be
>> pretty easy.
>
> Yeah, this did turn out to be really easy. I spent most of the time
> trying to explain the issue in the commit message in a sane way.
> Hopefully it didn't end up _too_ long. :)
>
> The interesting bit is in the third one. The first is a necessary
> preparatory step, and the second is a cleanup I noticed in the
> neighborhood.
>
> [1/3]: execv_dashed_external: use child_process struct
> [2/3]: execv_dashed_external: stop exiting with negative code
> [3/3]: execv_dashed_external: wait for child on signal death
>
> git.c | 36 +++++++++++++++---------------------
> run-command.c | 19 +++++++++++++++++++
> run-command.h | 1 +
> 3 files changed, 35 insertions(+), 21 deletions(-)
>
> -Peff
I don't see the rest of the patches on the list..?
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`
From: brian m. carlson @ 2017-01-07 22:08 UTC (permalink / raw)
To: Jeff King
Cc: Lars Schneider, Johannes Schindelin, git,
마누엘, Junio C Hamano
In-Reply-To: <20170105164556.b3bzeqqzx4pvni4z@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]
On Thu, Jan 05, 2017 at 11:45:57AM -0500, Jeff King wrote:
> On Thu, Jan 05, 2017 at 11:05:29AM +0100, Lars Schneider wrote:
>
> > > The git-scm.com site uses asciidoctor, too, and I think I have seen some
> > > oddness with the rendering though. So in general I am in favor of making
> > > things work under both asciidoc and asciidoctor.
> >
> > I am not familiar with both tools but it sounds to me as if "asciidoctor"
> > is kind of the "lowest common denominator". Is this true? If yes, would it
> > make sense to switch TravisCI [1] to asciidocter if this change gets merged?
>
> I don't think that's quite true.
>
> The two programs produce different output for certain inputs. We tend to
> see the cases where asciidoc produces the desired output and asciidoctor
> doesn't, because traditionally the documentation was written _for_
> asciidoc. So whenever asciidoctor diverges, it looks like a bug.
This is indeed the case. Asciidoctor is a bit stricter on some inputs
because it provides significant performance improvements, but it also
has features that AsciiDoc does not. It also has some bugs that
AsciiDoc does not (again, usually due to performance concerns).
For example, Debian's reproducible builds project would probably like it
if we had better support for Asciidoctor.
> [1] I think we've also traditionally considered asciidoc to be the
> definitive toolchain, and people using asciidoctor are free to
> submit patches to make things work correctly in both places. I'm not
> opposed to changing that attitude, as it seems like asciidoctor is
> faster and more actively maintained these days. But I suspect our
> build chain would need some improvements. Last time I tried building
> with AsciiDoctor it involved a lot manual tweaking of Makefile
> variables. It sounds like Dscho is doing it regularly, though. It
> should probably work out of the box (with something like
> USE_ASCIIDOCTOR=Yes) if we expect people to actually rely on it.
Yes, that would probably be beneficial. I'll see if I can come up with
some patches based on Dscho's work.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] giteveryday: unbreak rendering with AsciiDoctor
From: Junio C Hamano @ 2017-01-07 22:05 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20170104081544.5htofa3zpgkvty7x@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Jan 02, 2017 at 05:04:05PM +0100, Johannes Schindelin wrote:
>
>> The "giteveryday" document has a callout list that contains a code
>> block. This is not a problem for AsciiDoc, but AsciiDoctor sadly was
>> explicitly designed *not* to render this correctly [*1*]. The symptom is
>> an unhelpful
>>
>> line 322: callout list item index: expected 1 got 12
>> line 325: no callouts refer to list item 1
>> line 325: callout list item index: expected 2 got 13
>> line 327: no callouts refer to list item 2
>>
>> In Git for Windows, we rely on the speed improvement of AsciiDoctor (on
>> this developer's machine, `make -j15 html` takes roughly 30 seconds with
>> AsciiDoctor, 70 seconds with AsciiDoc), therefore we need a way to
>> render this correctly.
>>
>> The easiest way out is to simplify the callout list, as suggested by
>> AsciiDoctor's author, even while one may very well disagree with him
>> that a code block hath no place in a callout list.
>
> This looks like a good re-write to avoid the issue while maintaining the
> meaning and flow of the original.
OK. Ack.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`
From: Junio C Hamano @ 2017-01-07 22:03 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git, 마누엘
In-Reply-To: <20170104080852.bmlmtzxhjx4qt74f@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Jan 02, 2017 at 05:03:57PM +0100, Johannes Schindelin wrote:
>
>> From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= <nalla@hamal.uberspace.de>
>>
>> The `user-manual.txt` is designed as a `book` but the `Makefile` wants
>> to build it as an `article`. This seems to be a problem when building
>> the documentation with `asciidoctor`. Furthermore the parts *Git
>> Glossary* and *Appendix B* had no subsections which is not allowed when
>> building with `asciidoctor`. So lets add a *dummy* section.
>
> The git-scm.com site uses asciidoctor, too, and I think I have seen some
> oddness with the rendering though. So in general I am in favor of making
> things work under both asciidoc and asciidoctor.
>
> I diffed the results of "make user-manual.html" before and after this
> patch. A lot of "h3" chapter titles get bumped to "h2", which is OK. The
> chapter titles now say "Chapter 1. Repositories and Branches" rather
> than just "Repositories and Branches". Likewise, references now say
>
> Chapter 1, _Repositories and Branches_
>
> rather than:
>
> the section called "Repositories and Branches".
>
> which is probably OK, though the whole thing is short enough that
> calling the sections chapters feels a bit over-important.
Is that a longer way to say that the claim "... is designed as a
book" is false?
> So I dunno. I really do think "article" is conceptually the most
> appropriate style, but I agree that there are some book-like things
> (like appendices).
... Yeah, I should have read forward first before starting to insert
my comments.
^ permalink raw reply
* Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`
From: Junio C Hamano @ 2017-01-07 22:00 UTC (permalink / raw)
To: Jeff King
Cc: Lars Schneider, Johannes Schindelin, git,
마누엘
In-Reply-To: <20170105164556.b3bzeqqzx4pvni4z@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> As far as CI goes, I am not altogether convinced of the usefulness of
> building the documentation. It's very expensive, and the failure mode is
> rarely "whoops, running `make doc` failed". It's almost always that the
> output looks subtly wrong, but that's very hard to check automatically.
I've seen "make doc" break for me when it works for other people,
and if CI can check the formatting for various combinations of
versions of toolchain components, it would have some value. But
otherwise, I agree that "make doc" in CI would not give us as much
benefit as we spend electricity on it.
If we are to make support for building with AsciiDoctor better, CI
that does "make doc" with both AsciiDoc and AsciiDoctor might be of
help---a "make it buildable with A" patch may accidentally break B
and vice versa.
> [1] I think we've also traditionally considered asciidoc to be the
> definitive toolchain, and people using asciidoctor are free to
> submit patches to make things work correctly in both places. I'm not
> opposed to changing that attitude, as it seems like asciidoctor is
> faster and more actively maintained these days. But I suspect our
> build chain would need some improvements. Last time I tried building
> with AsciiDoctor it involved a lot manual tweaking of Makefile
> variables. It sounds like Dscho is doing it regularly, though. It
> should probably work out of the box (with something like
> USE_ASCIIDOCTOR=Yes) if we expect people to actually rely on it.
I do not mind getting convinced to declare that we will swap the
positions of these two documentation toolchains in some future date,
and a good first step for it to happen would be an easier out-of-box
experience.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 05/34] sequencer (rebase -i): learn about the 'verbose' mode
From: Junio C Hamano @ 2017-01-07 21:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1701021610590.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Junio,
>
> On Tue, 13 Dec 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>> > @@ -1493,9 +1498,26 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>> > }
>> >
>> > if (is_rebase_i(opts)) {
>> > + struct strbuf buf = STRBUF_INIT;
>> > +
>> > /* Stopped in the middle, as planned? */
>> > if (todo_list->current < todo_list->nr)
>> > return 0;
>> > +
>> > + if (opts->verbose) {
>> > + const char *argv[] = {
>> > + "diff-tree", "--stat", NULL, NULL
>> > + };
>> > +
>> > + if (!read_oneliner(&buf, rebase_path_orig_head(), 0))
>> > + return error(_("could not read '%s'"),
>> > + rebase_path_orig_head());
>> > + strbuf_addstr(&buf, "..HEAD");
>> > + argv[2] = buf.buf;
>> > + run_command_v_opt(argv, RUN_GIT_CMD);
>> > + strbuf_reset(&buf);
>> > + }
>> > + strbuf_release(&buf);
>> > }
>>
>> It's a bit curious that the previous step avoided running a separate
>> process and instead did "diff-tree -p" all in C, but this one does not.
>
> I guess my only defence is that I tried to be a little lazy.
I actually was alluding to going the other way around, spawning
"diff-tree -p" in the other codepath like this one does.
^ permalink raw reply
* Re: "git fsck" not detecting garbage at the end of blob object files...
From: Dennis Kaarsemaker @ 2017-01-07 21:47 UTC (permalink / raw)
To: John Szakmeister, git
In-Reply-To: <CAEBDL5Uc39JagdmXUxfxh1TPSK3H5wxoTfjK-pfLRYjciBnHpA@mail.gmail.com>
On Sat, 2017-01-07 at 07:50 -0500, John Szakmeister wrote:
> I was perusing StackOverflow this morning and ran across this
> question: http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/
>
> It was a simple question about why "checking objects" was not
> appearing, but in it was another issue. The user purposefully
> corrupted a blob object file to see if `git fsck` would catch it by
> tacking extra data on at the end. `git fsck` happily said everything
> was okay, but when I played with things locally I found out that `git
> gc` does not like that extra garbage. I'm not sure what the trade-off
> needs to be here, but my expectation is that if `git fsck` says
> everything is okay, then all operations using that object (file)
> should work too.
>
> Is that unreasonable? What would be the impact of fixing this issue?
If you do this with a commit object or tree object, fsck does complain.
I think it's sensible to do so for blob objects as well.
Editing blob object:
hurricane:/tmp/moo (master)$ hexer .git/objects/a1/b3ebb97f10ff8d85a9472bcba50cb575dbd485
hurricane:/tmp/moo (master)$ git status
On branch master
nothing to commit, working tree clean
hurricane:/tmp/moo (master)$ git fsck
Checking object directories: 100% (256/256), done.
hurricane:/tmp/moo (master)$ git gc
Counting objects: 3, done.
error: garbage at end of loose object 'a1b3ebb97f10ff8d85a9472bcba50cb575dbd485'
fatal: loose object a1b3ebb97f10ff8d85a9472bcba50cb575dbd485 (stored in .git/objects/a1/b3ebb97f10ff8d85a9472bcba50cb575dbd485) is corrupt
error: failed to run repack
Editing tree object:
hurricane:/tmp/moo (master)$ hexer .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20
hurricane:/tmp/moo (master +)$ git status
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
hurricane:/tmp/moo (master +)$ git fsck
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
Editing commit object:
hurricane:/tmp/moo (master)$ echo test >> .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0
hurricane:/tmp/moo (master +)$ git status
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
!(128) hurricane:/tmp/moo (master +)$ git fsck
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
D.
^ permalink raw reply
* Re: [PATCH v3 14/21] read-cache: touch shared index files when used
From: Junio C Hamano @ 2017-01-07 21:46 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <CAP8UFD1EgOxcPi=tpiosKkYMcCZe+b6gwW0CKt2sE1NZ7gQv=A@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> So what should we do if freshen_file() returns 0 which means that the
> freshening failed?
You tell me ;-) as you are the one who is proposing this feature.
Isn't a failure to freshen it a grave error? We are letting a
base/shared index file that is known to be in-use go stale and
eventually subject for garbage collection, and the user should be
notified in some way before the actual GC happens that renders the
index file unusable?
What is the failure mode after such a premature GC happens? What
does the end-user see? Can you try to (1) split the index (2)
modify bunch of entries (3) remove the base/shared index with /bin/rm
and then see how various Git commands fail? Do they fail gracefully?
I am trying to gauge the seriousness of ignoring such an error here.
^ permalink raw reply
* [PATCH] Makefile: POSIX windres
From: Steven Penny @ 2017-01-07 21:41 UTC (permalink / raw)
To: git; +Cc: Steven Penny
When environment variable POSIXLY_CORRECT is set, the "input -o output" syntax
is not supported.
http://cygwin.com/ml/cygwin/2017-01/msg00036.html
Signed-off-by: Steven Penny <svnpenn@gmail.com>
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index d861bd9..a2a1212 100644
--- a/Makefile
+++ b/Makefile
@@ -1816,7 +1816,7 @@ $(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
git.res: git.rc GIT-VERSION-FILE
$(QUIET_RC)$(RC) \
$(join -DMAJOR= -DMINOR=, $(wordlist 1,2,$(subst -, ,$(subst ., ,$(GIT_VERSION))))) \
- -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" $< -o $@
+ -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" -i $< -o $@
# This makes sure we depend on the NO_PERL setting itself.
$(SCRIPT_PERL_GEN): GIT-BUILD-OPTIONS
--
2.8.3
^ permalink raw reply related
* Re: [PATCH v3 08/21] Documentation/git-update-index: talk about core.splitIndex config var
From: Junio C Hamano @ 2017-01-07 21:38 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <CAP8UFD06mxGiZmr4Lwv3M8CedBZaamswzz-Q+mOxuuUFet8KNQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> It feels strange that when I do things one way, you suggest another
> way, and the next time in a similar situation when I do things the way
> you suggested previously, then you suggest the way I did it initially
> the first time...
Perhaps because neither is quite satisfactory and I am being forced
to choose between the two unsatifactory designs? In any case, I
mostly am and was pointing out the issues and not saying the other
one is the most preferred solution in these threads.
I think there should just be one authoritative source of the truth,
and I have always thought it should be the bit set in the index file
when the command line option is used, because that was the way the
feature was introduced first and I am superstitious about end-user
inertia. And from that point of view, no matter how you make this
new "config" thing interact with it, it would always give a strange
and unsatifactory end-user experience, at least to me.
Perhaps we should declare that config will be the one and only way
in the future and start deprecating the command line option way.
That will remove the need for two to interact with each other.
I dunno.
^ permalink raw reply
* Re: [PATCH] don't use test_must_fail with grep
From: Junio C Hamano @ 2017-01-07 21:18 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Johannes Sixt, Luke Diamand, Git Users, Stefan Beller
In-Reply-To: <CAFZEwPNbtamFfFy7vYXurpEWBDmRMyPB9+Ep-hm4uZVMREbq5Q@mail.gmail.com>
Pranit Bauva <pranit.bauva@gmail.com> writes:
> Hey Johannes,
>
> On Sun, Jan 1, 2017 at 8:20 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> which makes me wonder: Is the message that we do expect not to occur
>> actually printed on stdout? It sounds much more like an error message, i.e.,
>> text that is printed on stderr. Wouldn't we need this?
>>
>> git p4 commit >actual 2>&1 &&
>> ! grep "git author.*does not match" actual &&
>>
>> -- Hannes
>
> This seems better! Since I am at it, I can remove the traces of pipes
> in an another patch.
>
> Regards,
> Pranit Bauva
I see v3 that has 2>&1 but according to Luke's comment ("the message
comes from cat"), it shouldn't be there? I am behind clearing the
backlog in my mailbox and I could tweak it out from v3 while
queuing, or I may forget about it after looking at other topics ;-)
in which case you may want to send v4 with the fix?
^ permalink raw reply
* Re: RFC: --force-with-lease default behaviour
From: Junio C Hamano @ 2017-01-07 21:04 UTC (permalink / raw)
To: G. Sylvie Davies; +Cc: git
In-Reply-To: <CAAj3zPx4uMXhV7t86Cnn8SgmpXb2SGththYN7sHetOqL_JosMg@mail.gmail.com>
"G. Sylvie Davies" <sylvie@bit-booster.com> writes:
> I wonder if there's anything one could do to help those who type "git
> fetch" and still want to enjoy "--force-with-lease"...
The entire idea behind "force-with-lease" is that you plan to later
force update the tip of a branch at the remote to replace the commit
that used to be at the tip at some point, that you do not want other
people to have their own work on that branch that will be lost by
your later force-pushing, yet you cannot "lock" a branch at the
remote repository remotely because that goes against the distributed
nature of the development. Instead of locking others out, forcing
others to wait and sit idle while you complete the material to be
force-pushed (which may never happen), you base your work on one
state of the remote branch, and make sure the remote branch hasn't
advanced in the meantime (or you redo your work)---the cost of the
extra work due to your planned force-pushing is beared by you, not
by others.
There however is no place in Git where you explicitly declare "this
is where I start working on producing a new commit. That commit
will replace this state and will not fast-forward from it." and
store it locally. The "--force-with-lease" was designed to take
that information from the command line, expecting that the script
that drives it does something like
#!/bin/sh
LEASE=$(git rev-parse --verify @{u})
# do whatever that requires non-fast-forward push
git commit --amend ...
... maybe more ...
# finally push it out
git push --force-with-lease $LEASE ...
Lazy people decided that as long as they promise to themselves that
they are not going to do anything to cause @{u} to move, they can
use it as a lazy-man's approximate. Perhaps that was a misguided
attempt to add convenience.
A possible answer to your wordering may be to deprecate the
defaulting to @{u} and always require the expected commit to be
specified explicitly.
^ 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