* [PATCH] Allow stashes to be referenced by index only
From: Aaron M Watson @ 2016-09-08 23:46 UTC (permalink / raw)
To: git
Cc: Aaron M Watson, Jon Seymour, David Caldwell, Øystein Walle,
Jeff King, Ævar Arnfjörð Bjarmason, David Aguilar,
Alex Henrie
Instead of referencing "stash@{n}" explicitly, it can simply be
referenced as "n". Most users only reference stashes by their position
in the stash stask (what I refer to as the "index"). The syntax for the
typical stash (stash@{n}) is slightly annoying and easy to forget, and
sometimes difficult to escape properly in a script. Because of this the
capability to do things with the stash by simply referencing the index
is desirable.
This patch includes the superior implementation provided by Øsse Walle
(thanks for that), with a slight change to fix a broken test in the test
suite. I also merged the test scripts as suggested by Jeff King, and
un-wrapped the documentation as suggested by Junio Hamano.
Signed-off-by: Aaron M Watson <watsona4@gmail.com>
---
Documentation/git-stash.txt | 3 ++-
git-stash.sh | 17 +++++++++++++++--
t/t3903-stash.sh | 35 +++++++++++++++++++++++++++++++++++
3 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 92df596..2e9cef0 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -39,7 +39,8 @@ The latest stash you created is stored in `refs/stash`; older
stashes are found in the reflog of this reference and can be named using
the usual reflog syntax (e.g. `stash@{0}` is the most recently
created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}`
-is also possible).
+is also possible). Stashes may also be referenced by specifying just the
+stash index (e.g. the integer `n` is equivalent to `stash@{n}`).
OPTIONS
-------
diff --git a/git-stash.sh b/git-stash.sh
index 826af18..d8d3b8d 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -384,9 +384,10 @@ parse_flags_and_rev()
i_tree=
u_tree=
- REV=$(git rev-parse --no-flags --symbolic --sq "$@") || exit 1
+ REV=$(git rev-parse --no-flags --symbolic --sq "$@" 2> /dev/null)
FLAGS=
+ ARGV=
for opt
do
case "$opt" in
@@ -404,10 +405,13 @@ parse_flags_and_rev()
die "$(eval_gettext "unknown option: \$opt")"
FLAGS="${FLAGS}${FLAGS:+ }$opt"
;;
+ *)
+ ARGV="${ARGV}${ARGV:+ }'$opt'"
+ ;;
esac
done
- eval set -- $REV
+ eval set -- $ARGV
case $# in
0)
@@ -422,6 +426,15 @@ parse_flags_and_rev()
;;
esac
+ case "$1" in
+ *[!0-9]*)
+ :
+ ;;
+ *)
+ set -- "${ref_stash}@{$1}"
+ ;;
+ esac
+
REV=$(git rev-parse --symbolic --verify --quiet "$1") || {
reference="$1"
die "$(eval_gettext "\$reference is not a valid reference")"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2142c1f..f82a8c4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -131,6 +131,26 @@ test_expect_success 'drop middle stash' '
test 1 = $(git show HEAD:file)
'
+test_expect_success 'drop middle stash by index' '
+ git reset --hard &&
+ echo 8 > file &&
+ git stash &&
+ echo 9 > file &&
+ git stash &&
+ git stash drop 1 &&
+ test 2 = $(git stash list | wc -l) &&
+ git stash apply &&
+ test 9 = $(cat file) &&
+ test 1 = $(git show :file) &&
+ test 1 = $(git show HEAD:file) &&
+ git reset --hard &&
+ git stash drop &&
+ git stash apply &&
+ test 3 = $(cat file) &&
+ test 1 = $(git show :file) &&
+ test 1 = $(git show HEAD:file)
+'
+
test_expect_success 'stash pop' '
git reset --hard &&
git stash pop &&
@@ -604,6 +624,21 @@ test_expect_success 'invalid ref of the form stash@{n}, n >= N' '
git stash drop
'
+test_expect_success 'invalid ref of the form "n", n >= N' '
+ git stash clear &&
+ test_must_fail git stash drop 0 &&
+ echo bar5 > file &&
+ echo bar6 > file2 &&
+ git add file2 &&
+ git stash &&
+ test_must_fail git stash drop 1 &&
+ test_must_fail git stash pop 1 &&
+ test_must_fail git stash apply 1 &&
+ test_must_fail git stash show 1 &&
+ test_must_fail git stash branch tmp 1 &&
+ git stash drop
+'
+
test_expect_success 'stash branch should not drop the stash if the branch exists' '
git stash clear &&
echo foo >file &&
--
2.7.4
^ permalink raw reply related
* Git Bash and Git GUI freezing just after opening
From: André Marcondes @ 2016-09-09 3:05 UTC (permalink / raw)
To: git
I have a fresh git installation on my windows 10 machine, but I am
unable to use the Git Bash or the Git GUI because the program freezes
just after opening.
The only difference I have made on my machine was to update Windows 10
Pro to it's latest version (build 14393.105) and change the default
Documents folder to another partition.
I really need to fix this. How can I get some help?
^ permalink raw reply
* Re: [PATCH] gpg-interface: reflect stderr to stderr
From: Johannes Schindelin @ 2016-09-09 7:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Michael J Gruber, git
In-Reply-To: <xmqqwpimgso6.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 8 Sep 2016, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Wed, Sep 07, 2016 at 10:27:34AM +0200, Michael J Gruber wrote:
> >
> >> Now, I can't reproduce C on Linux[*], so there is more involved. It
> >> could be that my patch just exposes a problem in our start_command()
> >> etc.: run-command.c contains a lot of ifdefing, so possibly quite
> >> different code is run on different platforms.
> >
> > Maybe, though my blind guess is that it is simply that on Linux we can
> > open /dev/tty directly, and console-IO on Windows is a bit more
> > complicated.
>
> True.
>
> Even though this patch is fixing only one of the two issues, I am
> tempted to say that we should queue it for now, as it does so
> without breaking a bigger gain made by the original, i.e. we learn
> the status of verification in a way the authors of GPG wants us to,
> while somebody figuires out what the best way is to show the prompt
> to the console on Windows.
Between protecting users from their own mis-configurations and allowing
them to enter their passphrase interactively on Windows, I would argue
that the more important thing to have working is the latter, because it is
not at all the user's fault that they cannot enter their passphrase
currently, and they cannot fix it by fixing their mis-configuration.
Unfortunately, you obviously disagree with this assessement.
As I *need* to fix this *major* bug for Windows users, you basically put
an additional burden on me by applying Michael's patch, which not only
does not fix the problem for Windows users, but conflicts with my
work-around.
Pity,
Dscho
^ permalink raw reply
* Re: [PATCH v2 2/3] Introduce a function to run regexec() on non-NUL-terminated buffers
From: Johannes Schindelin @ 2016-09-09 9:45 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20160908080447.adquu2e5d7bbeorn@sigill.intra.peff.net>
Hi Peff,
On Thu, 8 Sep 2016, Jeff King wrote:
> On Thu, Sep 08, 2016 at 09:31:11AM +0200, Johannes Schindelin wrote:
>
> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index db89ba7..19128b3 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -965,6 +965,27 @@ void git_qsort(void *base, size_t nmemb, size_t size,
> > #define qsort git_qsort
> > #endif
> >
> > +static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
> > + size_t nmatch, regmatch_t pmatch[], int eflags)
> > +{
> > +#ifdef REG_STARTEND
> > + assert(nmatch > 0 && pmatch);
> > + pmatch[0].rm_so = 0;
> > + pmatch[0].rm_eo = size;
> > + return regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);
> > +#else
> > + char *buf2 = xmalloc(size + 1);
> > + int ret;
> > +
> > + memcpy(buf2, buf, size);
> > + buf2[size] = '\0';
>
> I mentioned elsewhere that I'd prefer we just push people into using
> compat/regex if they don't have REG_STARTEND. But if we _do_ keep this
> fallback, note that the above has a buffer overflow (think what happens
> when "size" is the maximum value for a size_t). You can avoid it by
> using xmallocz().
That buffer overflow does not exist: If size were the maximum value for
size_t, then buf->ptr would point at a buffer that occupies the entire
available memory, meaning that there is no space left for buf->ptr, let
alone for buf.
But I get your point. It is better to be consistent and use the same logic
for *all* allocations.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function
From: Johannes Schindelin @ 2016-09-09 9:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <xmqq1t0ujp2f.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 8 Sep 2016, Junio C Hamano wrote:
> Please give these three patches a common prefix, e.g.
>
> regex: -G<pattern> feeds a non NUL-terminated string to regexec() and fails
> regex: add regexec_buf() that can work on a non NUL-terminated string
> regex: use regexec_buf()
>
> or something like that.
Done.
> Also I agree with Peff that a test with an embedded NUL would be a
> good thing.
This is something I will leave to somebody else, as it was not my
intention to fix this and I *really* have more pressing things to do right
now... Sorry!
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function
From: Jeff King @ 2016-09-09 9:57 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.DEB.2.20.1609091151510.129229@virtualbox>
On Fri, Sep 09, 2016 at 11:52:50AM +0200, Johannes Schindelin wrote:
> > Also I agree with Peff that a test with an embedded NUL would be a
> > good thing.
>
> This is something I will leave to somebody else, as it was not my
> intention to fix this and I *really* have more pressing things to do right
> now... Sorry!
I think it is literally just squashing this into your final patch:
diff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh
index f0bf50b..37b8dde 100755
--- a/t/t4061-diff-pickaxe.sh
+++ b/t/t4061-diff-pickaxe.sh
@@ -19,4 +19,13 @@ test_expect_success '-G matches' '
test 4096-zeroes.txt = "$(cat out)"
'
+test_expect_success '-G matches after embedded NUL' '
+ printf "one\0two" >file &&
+ git add file &&
+ git commit -m embedded &&
+ echo embedded >expect &&
+ git log -Gtwo --format=%s >actual &&
+ test_cmp expect actual
+'
+
test_done
-Peff
^ permalink raw reply related
* Re: [PATCH] t6026-merge-attr: wait for process to release trash directory
From: Johannes Schindelin @ 2016-09-09 9:58 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Johannes Sixt, Git Mailing List
In-Reply-To: <20160908082713.sgzp2evbvzuthdb4@sigill.intra.peff.net>
Hi Peff,
On Thu, 8 Sep 2016, Jeff King wrote:
> On Thu, Sep 08, 2016 at 10:05:58AM +0200, Johannes Schindelin wrote:
>
> > > > Is fifo safe on Windows, though?
> > >
> > > No clue. We seem to use mkfifo unconditionally in lib-daemon, but
> > > perhaps people do not run that test on Windows. Other invocations seem
> > > to be protected by the PIPE prerequisite. But...
> >
> > AFAICT we do not use mkfifo on Windows. Let's see what t/test-lib.sh has
> > to say about the matter:
> >
> > test_lazy_prereq PIPE '
> > # test whether the filesystem supports FIFOs
> > case $(uname -s) in
> > CYGWIN*|MINGW*)
> > false
> > ;;
> > *)
> > rm -f testfifo && mkfifo testfifo
> > ;;
> > esac
> > '
> >
> > So there you go.
> >
> > The reason it is disabled is that Cygwin/MSYS2 do have a concept of a
> > FIFO. But `git.exe` won't be able to access such a FIFO because it is
> > emulated by the POSIX emulation layer, which Git cannot access.
>
> Regarding my "unconditionally" above: coincidentally, I happened to be
> looking in lib-git-daemon.sh about an hour ago and noticed that we do
> indeed check "test_have_prereq PIPE" (just not near the mkfifo, of
> course, because we are not in a test block).
>
> It seems to have been added by a "Johannes Schindelin". Any relation?
Maybe ;-)
Dscho
^ permalink raw reply
* Re: [PATCH v2 2/3] Introduce a function to run regexec() on non-NUL-terminated buffers
From: Jeff King @ 2016-09-09 9:59 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609091140420.129229@virtualbox>
On Fri, Sep 09, 2016 at 11:45:01AM +0200, Johannes Schindelin wrote:
> > I mentioned elsewhere that I'd prefer we just push people into using
> > compat/regex if they don't have REG_STARTEND. But if we _do_ keep this
> > fallback, note that the above has a buffer overflow (think what happens
> > when "size" is the maximum value for a size_t). You can avoid it by
> > using xmallocz().
>
> That buffer overflow does not exist: If size were the maximum value for
> size_t, then buf->ptr would point at a buffer that occupies the entire
> available memory, meaning that there is no space left for buf->ptr, let
> alone for buf.
True. I fixed quite a lot of these last summer, but they are only really
dangerous when we have not already allocated the buffer.
> But I get your point. It is better to be consistent and use the same logic
> for *all* allocations.
Yep. Also, it is easier to audit if you do not have to trace back and
see that even though we do overflow the argument to malloc, it can't
happen because of memory constraints (this one is fairly obvious, but
quite a few that I fixed previously involved complicated reasoning about
how much RAM you could use).
-Peff
^ permalink raw reply
* Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Johannes Schindelin @ 2016-09-09 10:09 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20160908080035.czwn5y3re5bp5vkg@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2506 bytes --]
Hi Peff,
On Thu, 8 Sep 2016, Jeff King wrote:
> On Thu, Sep 08, 2016 at 09:29:58AM +0200, Johannes Schindelin wrote:
>
> > sorry for the late answer, I was really busy trying to come up with a new
> > and improved version of the patch series, and while hunting a bug I
> > introduced got bogged down with other tasks.
>
> No problem. I am not in a hurry.
I kind of am. The second half of September, I won't be able to do much of
anything Git-related, and this is a major bug that blocks some important
work.
So I kind of have to press on that front.
> I think I'd rather just have:
>
> #ifndef REG_STARTEND
> #error "Your regex library sucks. Compile with NO_REGEX=NeedsStartEnd"
> #endif
Done. Although I permitted myself to reword this a little ;-)
> One other question about REG_STARTEND is: what does it do with NULs
> inside the buffer? Certainly glibc (and our compat/regex) treat it as a
> buffer with a particular length and ignore embedded NULs, as we want.
> But the NetBSD documentation says only:
>
> REG_STARTEND The string is considered to start at string +
> pmatch[0].rm_so and to have a terminating NUL
> located at string + pmatch[0].rm_eo (there need not
> actually be a NUL at that location),
>
> Besides avoiding a segfault, one of the benefits of regcomp_buf() is
> that we will now find pickaxe-regex strings inside mixed binary/text
> files. But it's not clear to me that NetBSD's implementation does this.
>
> I guess we can assume it is fine (it is certainly no _worse_ than the
> current behavior), and if people's platforms do not handle it, they can
> build with NO_REGEX.
René mentioned in f96e567 (grep: use REG_STARTEND for all matching if
available, 2010-05-22) something along the lines of REG_STARTEND being
able to parse beyond NULs. My interpretation of NetBSD's documentation
agrees with your interpretation, though, that the buffers are still
thought of as being NUL-terminated, even if rm_eo makes the code *not*
look at that particular NUL.
Be that as it may: it is completely outside the purpose of my patch series
to take care of making it possible for Git's regex functions to match
buffers with embedded NULs. The only purpose of my patch series is to fix
the crash that was reported to me due to regexec() reading past a mmap()ed
buffer. I already let myself being talked into fixing more things than
that, and I have to leave it at that.
Ciao,
Dscho
^ permalink raw reply
* [PATCH v3 1/4] cat-file: fix a grammo in the man page
From: Johannes Schindelin @ 2016-09-09 10:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Torsten Bögershausen, Jeff King
In-Reply-To: <cover.1473415827.git.johannes.schindelin@gmx.de>
"... has be ..." -> "... has to be ..."
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/git-cat-file.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 18d03d8..071029b 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -54,8 +54,9 @@ OPTIONS
--textconv::
Show the content as transformed by a textconv filter. In this case,
- <object> has be of the form <tree-ish>:<path>, or :<path> in order
- to apply the filter to the content recorded in the index at <path>.
+ <object> has to be of the form <tree-ish>:<path>, or :<path> in
+ order to apply the filter to the content recorded in the index at
+ <path>.
--batch::
--batch=<format>::
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 0/4] cat-file: optionally convert to worktree version
From: Johannes Schindelin @ 2016-09-09 10:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Torsten Bögershausen, Jeff King
In-Reply-To: <cover.1472041389.git.johannes.schindelin@gmx.de>
When third-party tools need to access to contents of blobs in the
database, they might be more interested in the worktree version than in
the "clean" version of said contents.
This branch introduces the --filters option to make that happen, the
--use-path option to provide the path separately if the blob name rather
than the tree name is availale, and offers batch support in which case
it expects the object names and the path on its input lines, separated
by white space.
The new --filters option is an obvious sibling of --textconv, and shares
the peculiar feature that the drivers (and end-of-line convention) are
determined from the current worktree, not from the attributes stored in
the revision that may have been part of the object name.
As --textconv is so similar to --filters, it was taught to understand
the --use-path option and it was made compatible with batch mode, too.
I briefly considered teaching the batch mode to extract the path from
object names if they are specified as <tree-ish>:<path>. The changes
would be quite intrusive, though, and uglify the code substanitially. So
I decided against that.
Changes vs v2:
- always override unknown mode to imply 0100644, even if the user did
not specify the --path option.
Johannes Schindelin (4):
cat-file: fix a grammo in the man page
cat-file: introduce the --filters option
cat-file --textconv/--filters: allow specifying the path separately
cat-file: support --textconv/--filters in batch mode
Documentation/git-cat-file.txt | 40 +++++++++++----
builtin/cat-file.c | 110 ++++++++++++++++++++++++++++++++++++++---
t/t8010-cat-file-filters.sh | 64 ++++++++++++++++++++++++
3 files changed, 196 insertions(+), 18 deletions(-)
create mode 100755 t/t8010-cat-file-filters.sh
Published-As: https://github.com/dscho/git/releases/tag/cat-file-filters-v3
Fetch-It-Via: git fetch https://github.com/dscho/git cat-file-filters-v3
Interdiff vs v2:
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f8a3a08..4461153 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -71,7 +71,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
if (!path)
path = obj_context.path;
- else if (obj_context.mode == S_IFINVALID)
+ if (obj_context.mode == S_IFINVALID)
obj_context.mode = 0100644;
buf = NULL;
--
2.10.0.windows.1.10.g803177d
base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b
^ permalink raw reply
* [PATCH v3 3/4] cat-file --textconv/--filters: allow specifying the path separately
From: Johannes Schindelin @ 2016-09-09 10:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Torsten Bögershausen, Jeff King
In-Reply-To: <cover.1473415827.git.johannes.schindelin@gmx.de>
There are circumstances when it is relatively easy to figure out the
object name for a given path, but not the name of the containing tree.
For example, when looking at a diff generated by Git, the object names
are recorded, but not the revision. As a matter of fact, the revisions
from which the diff was generated may not even exist locally.
In such a case, the user would have to generate a fake revision just to
be able to use --textconv or --filters.
Let's simplify this dramatically, because we do not really need that
revision at all: all we care about is that we know the path. In the
scenario described above, we do know the path, and we just want to
specify it separately from the object name.
Example usage:
git cat-file --textconv --path=main.c 0f1937fd
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/git-cat-file.txt | 7 ++++++-
builtin/cat-file.c | 26 +++++++++++++++++++++-----
t/t8010-cat-file-filters.sh | 20 ++++++++++++++++++++
3 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 537d02c..4fa9041 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -9,7 +9,7 @@ git-cat-file - Provide content or type and size information for repository objec
SYNOPSIS
--------
[verse]
-'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) <object>
+'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--path=<path>] <object>
'git cat-file' (--batch | --batch-check) [--follow-symlinks]
DESCRIPTION
@@ -64,6 +64,11 @@ OPTIONS
end-of-line conversion, etc). In this case, <object> has to be of
the form <tree-ish>:<path>, or :<path>.
+--path=<path>::
+ For use with --textconv or --filters, to allow specifying an object
+ name and a path separately, e.g. when it is difficult to figure out
+ the revision from which the blob came.
+
--batch::
--batch=<format>::
Print object information and contents for each object provided
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 0b74afa..b648056 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -20,6 +20,8 @@ struct batch_options {
const char *format;
};
+static const char *force_path;
+
static int filter_object(const char *path, unsigned mode,
const unsigned char *sha1,
char **buf, unsigned long *size)
@@ -58,6 +60,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
struct object_info oi = {NULL};
struct strbuf sb = STRBUF_INIT;
unsigned flags = LOOKUP_REPLACE_OBJECT;
+ const char *path = force_path;
if (unknown_type)
flags |= LOOKUP_UNKNOWN_OBJECT;
@@ -65,6 +68,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
die("Not a valid object name %s", obj_name);
+ if (!path)
+ path = obj_context.path;
+ if (obj_context.mode == S_IFINVALID)
+ obj_context.mode = 0100644;
+
buf = NULL;
switch (opt) {
case 't':
@@ -89,21 +97,22 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
return !has_sha1_file(sha1);
case 'w':
- if (!obj_context.path[0])
+ if (!path[0])
die("git cat-file --filters %s: <object> must be "
"<sha1:path>", obj_name);
- if (filter_object(obj_context.path, obj_context.mode,
+ if (filter_object(path, obj_context.mode,
sha1, &buf, &size))
return -1;
break;
case 'c':
- if (!obj_context.path[0])
+ if (!path[0])
die("git cat-file --textconv %s: <object> must be <sha1:path>",
obj_name);
- if (textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))
+ if (textconv_object(path, obj_context.mode,
+ sha1, 1, &buf, &size))
break;
case 'p':
@@ -477,7 +486,7 @@ static int batch_objects(struct batch_options *opt)
}
static const char * const cat_file_usage[] = {
- N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) <object>"),
+ N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) [--path=<path>] <object>"),
N_("git cat-file (--batch | --batch-check) [--follow-symlinks]"),
NULL
};
@@ -525,6 +534,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
N_("for blob objects, run textconv on object's content"), 'c'),
OPT_CMDMODE(0, "filters", &opt,
N_("for blob objects, run filters on object's content"), 'w'),
+ OPT_STRING(0, "path", &force_path, N_("blob"),
+ N_("use a specific path for --textconv/--filters")),
OPT_BOOL(0, "allow-unknown-type", &unknown_type,
N_("allow -s and -t to work with broken/corrupt objects")),
OPT_BOOL(0, "buffer", &batch.buffer_output, N_("buffer --batch output")),
@@ -567,6 +578,11 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
usage_with_options(cat_file_usage, options);
}
+ if (force_path && opt != 'c' && opt != 'w') {
+ error("--path=<path> needs --textconv or --filters");
+ usage_with_options(cat_file_usage, options);
+ }
+
if (batch.buffer_output < 0)
batch.buffer_output = batch.all_objects;
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index e466634..0d5c33e 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -31,4 +31,24 @@ test_expect_success 'cat-file --filters converts to worktree version' '
has_cr actual
'
+test_expect_success 'cat-file --filters --path=<path> works' '
+ sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+ git cat-file --filters --path=world.txt $sha1 >actual &&
+ has_cr actual
+'
+
+test_expect_success 'cat-file --textconv --path=<path> works' '
+ sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+ test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
+ git cat-file --textconv --path=hello.txt $sha1 >rot13 &&
+ test uryyb = "$(cat rot13 | remove_cr)"
+'
+
+test_expect_success '----path=<path> complains without --textconv/--filters' '
+ sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+ test_must_fail git cat-file --path=hello.txt blob $sha1 >actual 2>err &&
+ test ! -s actual &&
+ grep "path.*needs.*filters" err
+'
+
test_done
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 4/4] cat-file: support --textconv/--filters in batch mode
From: Johannes Schindelin @ 2016-09-09 10:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Torsten Bögershausen, Jeff King
In-Reply-To: <cover.1473415827.git.johannes.schindelin@gmx.de>
With this patch, --batch can be combined with --textconv or --filters.
For this to work, the input needs to have the form
<object name><single white space><path>
so that the filters can be chosen appropriately.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/git-cat-file.txt | 18 +++++++++++-----
builtin/cat-file.c | 49 +++++++++++++++++++++++++++++++++++++-----
t/t8010-cat-file-filters.sh | 10 +++++++++
3 files changed, 67 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 4fa9041..204541c 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) [--path=<path>] <object>
-'git cat-file' (--batch | --batch-check) [--follow-symlinks]
+'git cat-file' (--batch | --batch-check) [ --textconv | --filters ] [--follow-symlinks]
DESCRIPTION
-----------
@@ -20,7 +20,11 @@ object type, or `-s` is used to find the object size, or `--textconv` or
`--filters` is used (which imply type "blob").
In the second form, a list of objects (separated by linefeeds) is provided on
-stdin, and the SHA-1, type, and size of each object is printed on stdout.
+stdin, and the SHA-1, type, and size of each object is printed on stdout. The
+output format can be overridden using the optional `<format>` argument. If
+either `--textconv` or `--filters` was specified, the input is expected to
+list the object names followed by the path name, separated by a single white
+space, so that the appropriate drivers can be determined.
OPTIONS
-------
@@ -72,13 +76,17 @@ OPTIONS
--batch::
--batch=<format>::
Print object information and contents for each object provided
- on stdin. May not be combined with any other options or arguments.
- See the section `BATCH OUTPUT` below for details.
+ on stdin. May not be combined with any other options or arguments
+ except `--textconv` or `--filters`, in which case the input lines
+ also need to specify the path, separated by white space. See the
+ section `BATCH OUTPUT` below for details.
--batch-check::
--batch-check=<format>::
Print object information for each object provided on stdin. May
- not be combined with any other options or arguments. See the
+ not be combined with any other options or arguments except
+ `--textconv` or `--filters`, in which case the input lines also
+ need to specify the path, separated by white space. See the
section `BATCH OUTPUT` below for details.
--batch-all-objects::
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b648056..4461153 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -17,6 +17,7 @@ struct batch_options {
int print_contents;
int buffer_output;
int all_objects;
+ int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
const char *format;
};
@@ -285,7 +286,32 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
if (data->type == OBJ_BLOB) {
if (opt->buffer_output)
fflush(stdout);
- if (stream_blob_to_fd(1, sha1, NULL, 0) < 0)
+ if (opt->cmdmode) {
+ char *contents;
+ unsigned long size;
+
+ if (!data->rest)
+ die("missing path for '%s'", sha1_to_hex(sha1));
+
+ if (opt->cmdmode == 'w') {
+ if (filter_object(data->rest, 0100644, sha1,
+ &contents, &size))
+ die("could not convert '%s' %s",
+ sha1_to_hex(sha1), data->rest);
+ } else if (opt->cmdmode == 'c') {
+ enum object_type type;
+ if (!textconv_object(data->rest, 0100644, sha1,
+ 1, &contents, &size))
+ contents = read_sha1_file(sha1, &type,
+ &size);
+ if (!contents)
+ die("could not convert '%s' %s",
+ sha1_to_hex(sha1), data->rest);
+ } else
+ die("BUG: invalid cmdmode: %c", opt->cmdmode);
+ batch_write(opt, contents, size);
+ free(contents);
+ } else if (stream_blob_to_fd(1, sha1, NULL, 0) < 0)
die("unable to stream %s to stdout", sha1_to_hex(sha1));
}
else {
@@ -422,6 +448,8 @@ static int batch_objects(struct batch_options *opt)
data.mark_query = 1;
strbuf_expand(&buf, opt->format, expand_format, &data);
data.mark_query = 0;
+ if (opt->cmdmode)
+ data.split_on_whitespace = 1;
if (opt->all_objects) {
struct object_info empty;
@@ -487,7 +515,7 @@ static int batch_objects(struct batch_options *opt)
static const char * const cat_file_usage[] = {
N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) [--path=<path>] <object>"),
- N_("git cat-file (--batch | --batch-check) [--follow-symlinks]"),
+ N_("git cat-file (--batch | --batch-check) [--follow-symlinks] [--textconv|--filters]"),
NULL
};
@@ -558,7 +586,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);
if (opt) {
- if (argc == 1)
+ if (batch.enabled && (opt == 'c' || opt == 'w'))
+ batch.cmdmode = opt;
+ else if (argc == 1)
obj_name = argv[0];
else
usage_with_options(cat_file_usage, options);
@@ -570,8 +600,12 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
} else
usage_with_options(cat_file_usage, options);
}
- if (batch.enabled && (opt || argc)) {
- usage_with_options(cat_file_usage, options);
+ if (batch.enabled) {
+ if (batch.cmdmode != opt || argc)
+ usage_with_options(cat_file_usage, options);
+ if (batch.cmdmode && batch.all_objects)
+ die("--batch-all-objects cannot be combined with "
+ "--textconv nor with --filters");
}
if ((batch.follow_symlinks || batch.all_objects) && !batch.enabled) {
@@ -583,6 +617,11 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
usage_with_options(cat_file_usage, options);
}
+ if (force_path && batch.enabled) {
+ error("--path=<path> incompatible with --batch");
+ usage_with_options(cat_file_usage, options);
+ }
+
if (batch.buffer_output < 0)
batch.buffer_output = batch.all_objects;
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index 0d5c33e..acdfa09 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -51,4 +51,14 @@ test_expect_success '----path=<path> complains without --textconv/--filters' '
grep "path.*needs.*filters" err
'
+test_expect_success 'cat-file --textconv --batch works' '
+ sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
+ test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
+ printf "%s hello.txt\n%s hello\n" $sha1 $sha1 |
+ git cat-file --textconv --batch >actual &&
+ printf "%s blob 6\nuryyb\r\n\n%s blob 6\nhello\n\n" \
+ $sha1 $sha1 >expect &&
+ test_cmp expect actual
+'
+
test_done
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 2/4] cat-file: introduce the --filters option
From: Johannes Schindelin @ 2016-09-09 10:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Torsten Bögershausen, Jeff King
In-Reply-To: <cover.1473415827.git.johannes.schindelin@gmx.de>
The --filters option applies the convert_to_working_tree() filter for
the path when showing the contents of a regular file blob object.
This feature comes in handy when a 3rd-party tool wants to work with
the contents of files from past revisions as if they had been checked
out, but without detouring via temporary files.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/git-cat-file.txt | 12 +++++++++---
builtin/cat-file.c | 41 ++++++++++++++++++++++++++++++++++++++++-
t/t8010-cat-file-filters.sh | 34 ++++++++++++++++++++++++++++++++++
3 files changed, 83 insertions(+), 4 deletions(-)
create mode 100755 t/t8010-cat-file-filters.sh
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 071029b..537d02c 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -9,15 +9,15 @@ git-cat-file - Provide content or type and size information for repository objec
SYNOPSIS
--------
[verse]
-'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv ) <object>
+'git cat-file' (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv | --filters ) <object>
'git cat-file' (--batch | --batch-check) [--follow-symlinks]
DESCRIPTION
-----------
In its first form, the command provides the content or the type of an object in
the repository. The type is required unless `-t` or `-p` is used to find the
-object type, or `-s` is used to find the object size, or `--textconv` is used
-(which implies type "blob").
+object type, or `-s` is used to find the object size, or `--textconv` or
+`--filters` is used (which imply type "blob").
In the second form, a list of objects (separated by linefeeds) is provided on
stdin, and the SHA-1, type, and size of each object is printed on stdout.
@@ -58,6 +58,12 @@ OPTIONS
order to apply the filter to the content recorded in the index at
<path>.
+--filters::
+ Show the content as converted by the filters configured in
+ the current working tree for the given <path> (i.e. smudge filters,
+ end-of-line conversion, etc). In this case, <object> has to be of
+ the form <tree-ish>:<path>, or :<path>.
+
--batch::
--batch=<format>::
Print object information and contents for each object provided
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 2dfe626..0b74afa 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -20,6 +20,33 @@ struct batch_options {
const char *format;
};
+static int filter_object(const char *path, unsigned mode,
+ const unsigned char *sha1,
+ char **buf, unsigned long *size)
+{
+ enum object_type type;
+
+ *buf = read_sha1_file(sha1, &type, size);
+ if (!*buf)
+ return error(_("cannot read object %s '%s'"),
+ sha1_to_hex(sha1), path);
+ if (type != OBJ_BLOB) {
+ free(*buf);
+ return error(_("blob expected for %s '%s'"),
+ sha1_to_hex(sha1), path);
+ }
+ if (S_ISREG(mode)) {
+ struct strbuf strbuf = STRBUF_INIT;
+ if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
+ free(*buf);
+ *size = strbuf.len;
+ *buf = strbuf_detach(&strbuf, NULL);
+ }
+ }
+
+ return 0;
+}
+
static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
int unknown_type)
{
@@ -61,6 +88,16 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
case 'e':
return !has_sha1_file(sha1);
+ case 'w':
+ if (!obj_context.path[0])
+ die("git cat-file --filters %s: <object> must be "
+ "<sha1:path>", obj_name);
+
+ if (filter_object(obj_context.path, obj_context.mode,
+ sha1, &buf, &size))
+ return -1;
+ break;
+
case 'c':
if (!obj_context.path[0])
die("git cat-file --textconv %s: <object> must be <sha1:path>",
@@ -440,7 +477,7 @@ static int batch_objects(struct batch_options *opt)
}
static const char * const cat_file_usage[] = {
- N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv) <object>"),
+ N_("git cat-file (-t [--allow-unknown-type]|-s [--allow-unknown-type]|-e|-p|<type>|--textconv|--filters) <object>"),
N_("git cat-file (--batch | --batch-check) [--follow-symlinks]"),
NULL
};
@@ -486,6 +523,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
OPT_CMDMODE('p', NULL, &opt, N_("pretty-print object's content"), 'p'),
OPT_CMDMODE(0, "textconv", &opt,
N_("for blob objects, run textconv on object's content"), 'c'),
+ OPT_CMDMODE(0, "filters", &opt,
+ N_("for blob objects, run filters on object's content"), 'w'),
OPT_BOOL(0, "allow-unknown-type", &unknown_type,
N_("allow -s and -t to work with broken/corrupt objects")),
OPT_BOOL(0, "buffer", &batch.buffer_output, N_("buffer --batch output")),
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
new file mode 100755
index 0000000..e466634
--- /dev/null
+++ b/t/t8010-cat-file-filters.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+test_description='git cat-file filters support'
+. ./test-lib.sh
+
+test_expect_success 'setup ' '
+ echo "*.txt eol=crlf diff=txt" >.gitattributes &&
+ echo "hello" | append_cr >world.txt &&
+ git add .gitattributes world.txt &&
+ test_tick &&
+ git commit -m "Initial commit"
+'
+
+has_cr () {
+ tr '\015' Q <"$1" | grep Q >/dev/null
+}
+
+test_expect_success 'no filters with `git show`' '
+ git show HEAD:world.txt >actual &&
+ ! has_cr actual
+
+'
+
+test_expect_success 'no filters with cat-file' '
+ git cat-file blob HEAD:world.txt >actual &&
+ ! has_cr actual
+'
+
+test_expect_success 'cat-file --filters converts to worktree version' '
+ git cat-file --filters HEAD:world.txt >actual &&
+ has_cr actual
+'
+
+test_done
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* Re: [PATCH 2/3] diff_flush_patch_id: stop returning error result
From: Johannes Schindelin @ 2016-09-09 10:28 UTC (permalink / raw)
To: Jeff King
Cc: git, Michael Haggerty, Kevin Willford, Xiaolong Ye, Josh Triplett
In-Reply-To: <20160907220409.oowxymhvkof2xsk5@sigill.intra.peff.net>
Hi Peff,
On Wed, 7 Sep 2016, Jeff King wrote:
> All of our errors come from diff_get_patch_id(), which has
> exactly three error conditions. The first is an internal
> assertion, which should be a die("BUG") in the first place.
>
> The other two are caused by an inability to two diff blobs,
> which is an indication of a serious problem (probably
> repository corruption). All the rest of the diff subsystem
> dies immediately on these conditions. By passing up the
> error, in theory we can keep going even if patch-id is
> unable to function. But in practice this means we may
> generate subtly wrong results (e.g., by failing to correlate
> two commits). Let's just die(), as we're better off making
> it clear to the user that their repository is not
> functional.
>
> As a result, we can simplify the calling code.
I like the simplification, but I *hate* the fact that the calling code has
*no way* to inform the user about the proper next steps.
You are touching code that is really quite at the bottom of a lot of call
chains. For example in the one of `git pull --rebase`. I just spent an
insane amount of time trying to make sure that this command will not
simply die() somewhere deep in the code, leaving the user puzzled.
Please see 3be18b4 (t5520: verify that `pull --rebase` shows the helpful
advice when failing, 2016-07-26) for more details.
A much better way, in my opinion, would be to introduce a new flag, say,
skip_merges, and pass that to the diff_flush_patch_id() function. You
could also consider consolidating that flag with the diff_header_only flag
into a "flags" argument via something like
enum diff_flush_patch_id {
DIFF_HEADER_ONLY = 1,
SKIP_MERGES = 2
}
But it is definitely not a good idea to reintroduce the bad practice of
die()ing deep down in library code. I know, you want proper exception
handling. We cannot have that. We use C. But die() is not a solution: it
introduces new problems.
Mind you: I agree that there are serious problems in the cases you
illustrated. But none of those problems give us license to leave the user
utterly puzzled by not even telling them what is going on: spouting
internals such as "unable to read files to diff" is *most definitely* not
helping users who simply want to run a `git pull --rebase`.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 2/3] t0001: work around the bug that reads config file before repo setup
From: Duy Nguyen @ 2016-09-09 10:32 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Michael J Gruber, Max Nordlund
In-Reply-To: <20160908200202.gmvpqrwwjavxmojb@sigill.intra.peff.net>
On Fri, Sep 9, 2016 at 3:02 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Sep 08, 2016 at 08:47:18PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> git-init somehow reads '.git/config' at current directory and sets
>> log_all_ref_updates based on this file. Because log_all_ref_updates is
>> not unspecified (-1) any more. It will not be written to the new repo's
>> config file (see create_default_files() function).
>>
>> This will affect our tests in the next patch as we will compare the
>> config file and expect that core.logallrefupdates is already set to true
>> by "git init main-worktree".
>
> This is a bug for more than worktrees, and is something I'm working on
> fixing
Great! test_expect_failure it is. But I'll make a separate patch,
independent from this series though.
--
Duy
^ permalink raw reply
* Re: [PATCH 3/3] init: do not set core.worktree more often than necessary
From: Duy Nguyen @ 2016-09-09 10:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Michael J Gruber, Max Nordlund
In-Reply-To: <xmqqbmzygoat.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 9, 2016 at 2:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> +/*
>> + * Return the first ".git" that we have encountered.
>> + * FIXME this function for not entirely correct because
>> + * setup_git_directory() and enter_repo() do not update first_git_dir
>> + * when they follow .git files. The function in its current state is
>> + * only suitable for "git init".
>> + */
>
> Would it be possible to move this to "init-db.c" then?
>
> The very first thing cmd_init_db() does to what is in the
> environment.c is to call set_git_dir() via set_git_dir_init() to
> tell it where the ".git" thing is, no? Can't that code remember the
> location itself, instead of adding code that is known not to be
> usable by other callers? That would help avoiding the future
> confusion.
Good idea. I was fixated on read_gitfile()m it didn't occur to me.
--
Duy
^ permalink raw reply
* Re: [PATCH 1/3] init: correct re-initialization from a linked worktree
From: Duy Nguyen @ 2016-09-09 10:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Michael J Gruber, Max Nordlund
In-Reply-To: <xmqqk2emgp2m.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 9, 2016 at 2:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> When 'git init' is called from a linked worktree, '.git' dir as the main
>> '.git' (i.e. $GIT_COMMON_DIR) and populate the whole repository skeleton
>> in there. It does not harm anything (*) but it is still wrong.
>
> -ECANNOTPARSE. Did you mean "... worktree, we treat '.git' dir as
> if it is the main '.git' ..."
Yep I accidentally a couple words there. Will add them back.
> or something entirely different?
--
Duy
^ permalink raw reply
* Re: [PATCH 2/3] diff_flush_patch_id: stop returning error result
From: Jeff King @ 2016-09-09 10:40 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Michael Haggerty, Kevin Willford, Xiaolong Ye, Josh Triplett
In-Reply-To: <alpine.DEB.2.20.1609091219350.129229@virtualbox>
On Fri, Sep 09, 2016 at 12:28:38PM +0200, Johannes Schindelin wrote:
> I like the simplification, but I *hate* the fact that the calling code has
> *no way* to inform the user about the proper next steps.
>
> You are touching code that is really quite at the bottom of a lot of call
> chains. For example in the one of `git pull --rebase`. I just spent an
> insane amount of time trying to make sure that this command will not
> simply die() somewhere deep in the code, leaving the user puzzled.
>
> Please see 3be18b4 (t5520: verify that `pull --rebase` shows the helpful
> advice when failing, 2016-07-26) for more details.
Yes, I agree that this is the opposite direction of libification. And I
agree that the current message is not very helpful.
But I am not sure that returning the error up the stack will actually
help somebody move forward. The reason these are all die() calls in the
rest of the diff code is that they are generally indicative of
unrecoverable repository corruption. So any advice does not really
depend on what operation you are performing; it is always "stop what you
are doing immediately, run fsck, and try to get the broken objects from
somebody else".
So IMHO, on balance this is not hurting anything.
> A much better way, in my opinion, would be to introduce a new flag, say,
> skip_merges, and pass that to the diff_flush_patch_id() function. You
> could also consider consolidating that flag with the diff_header_only flag
> into a "flags" argument via something like
diff_flush_patch_id() doesn't care about merges; that's too late. The
change has to happen in commit_patch_id(). And the problem is not one of
passing in "skip merges" (we _always_ want to skip merges). It is rather
distinguishing the reason that commit_patch_id() told us it did not fill
in the sha1: because it was an error, or because the patch id is
undefined (one triggers a die(), the other a silent continue).
I think I laid out that path already in the cover letter of the
original. If the consensus is that this is too ugly, I can implement
that approach.
-Peff
^ permalink raw reply
* Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function
From: Johannes Schindelin @ 2016-09-09 10:41 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20160909095735.zen2ynpsn7azqfss@sigill.intra.peff.net>
Hi Peff,
On Fri, 9 Sep 2016, Jeff King wrote:
> On Fri, Sep 09, 2016 at 11:52:50AM +0200, Johannes Schindelin wrote:
>
> > > Also I agree with Peff that a test with an embedded NUL would be a
> > > good thing.
> >
> > This is something I will leave to somebody else, as it was not my
> > intention to fix this and I *really* have more pressing things to do right
> > now... Sorry!
>
> I think it is literally just squashing this into your final patch:
>
> diff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh
> index f0bf50b..37b8dde 100755
> --- a/t/t4061-diff-pickaxe.sh
> +++ b/t/t4061-diff-pickaxe.sh
> @@ -19,4 +19,13 @@ test_expect_success '-G matches' '
> test 4096-zeroes.txt = "$(cat out)"
> '
>
> +test_expect_success '-G matches after embedded NUL' '
> + printf "one\0two" >file &&
> + git add file &&
> + git commit -m embedded &&
> + echo embedded >expect &&
> + git log -Gtwo --format=%s >actual &&
> + test_cmp expect actual
> +'
> +
> test_done
Thank you for providing me with the patch.
However, the whole idea of supporting regular expressions on buffers with
embedded NULs *is* different from the purpose of this patch series.
And in my quick web search, I got the impression that the presence of
REG_STARTEND really does not guarantee that regexec() won't stop at the
first NUL when rm_eo points after it.
So yeah, the patch would be easy to squash in, but the entire "rat's tail"
of making sure that this works everywhere, *in addition* to making sure
that the crash on mmap()ed buffers no longer occurs, would just delay this
patch series.
And unfortunately I do not have time for that right now.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix
From: Duy Nguyen @ 2016-09-09 10:43 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Jeff King, Junio C Hamano, Leho Kraav, Git Mailing List
In-Reply-To: <20160907174841.Horde.Ru1LBEeLKomznlWVG-ZnS-Q@webmail.informatik.kit.edu>
On Wed, Sep 7, 2016 at 10:48 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> Now, while I believe this is the right thing to do to fix this bug,
> there is a corner case, where multiple configured prerelease suffixes
> might match the same tagname:
>
> $ git config --get-all versionsort.prereleaseSuffix
> -bar
> -baz
> -foo-bar
> $ ~/src/git/git tag -l --sort=version:refname
> v1.0-foo-bar
> v1.0-foo-baz
>
> I.e. when comparing these two tags, both "-bar" and "-foo-bar" would
> match "v1.0-foo-bar", and as "-bar" comes first in the config file,
> it wins, and "v1.0-foo-bar" is ordered first. An argument could be
> made to prefer longer matches, in which case "v1.0-foo-bar" would be
> ordered according to "-foo-bar", i.e. as second. However, I don't
> know what that argument could be, to me neither behavior is better
> than the other, but the implementation of the "longest match counts"
> would certainly be more complicated.
>
> The argument I would make is that this is a pathological corner case
> that doesn't worth worrying about.
Maybe we should keep a note about this in config.txt? If it's not
worth bothering/scaring the users about, I suggest you keep this in
the commit message.
--
Duy
^ permalink raw reply
* Re: [PATCH 2/6] pull: make code more similar to the shell script again
From: Johannes Schindelin @ 2016-09-09 10:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqr3976vcy.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 29 Aug 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > +static int require_clean_work_tree(const char *action, const char *hint,
> > + int gently)
> > {
> > struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
> > - int do_die = 0;
> > + int err = 0;
> >
> > hold_locked_index(lock_file, 0);
> > refresh_cache(REFRESH_QUIET);
> > @@ -376,20 +377,26 @@ static void die_on_unclean_work_tree(void)
> > rollback_lock_file(lock_file);
> >
> > if (has_unstaged_changes()) {
> > - error(_("Cannot pull with rebase: You have unstaged changes."));
> > - do_die = 1;
> > + error(_("Cannot %s: You have unstaged changes."), action);
> > ...
> > if (!autostash)
> > - die_on_unclean_work_tree();
> > + require_clean_work_tree("pull with rebase",
> > + "Please commit or stash them.", 0);
> >
> > if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
> > hashclr(rebase_fork_point);
>
> Splicing an English/C phrase 'pull with rebase' into a
> _("localizable %s string") makes the life of i18n team hard.
>
> Can we do this differently?
>
> If you are eventually going to expose this function as public API, I
> think the right approach would be to enumerate the possible error
> conditions this function can diagnose and return them to the caller,
> i.e.
>
> #define WT_STATUS_DIRTY_WORKTREE 01
> #define WT_STATUS_DIRTY_INDEX 02
>
> static int require_clean_work_tree(void)
> {
> int status = 0;
> ...
> if (has_unstaged_changes())
> status |= WT_STATUS_DIRTY_WORKTREE;
> if (has_uncommitted_changes())
> status |= WT_STATUS_DIRTY_INDEX;
> return status;
> }
>
> Then die_on_unclean_work_tree() can be made as a thin-wrapper that
> calls it and shows the pull-specific error message.
Hrm. After thinking about this for over a week, I think that this is the
wrong approach.
To introduce new wrapper functions just for the sake of being able to
provide possibly dozens of different error messages seems quite a bit
wrong.
I agree, however, that it may be a better idea to add a "gently" flag to
require_clean_work_tree() that lets it print out a (localizable) error
message and return -1 instead of die()ing.
The result would be that a failed `git pull --rebase` would then print out
*two* lines: one explaining that there are unstaged changes, and one that
explains that the pull did not even start due to an unclean work tree.
That solution would scale better, and I may get a chance to make those
changes and send out another iteration of this patch series before
October.
Ciao,
Johannes
^ permalink raw reply
* Re: git commit -p with file arguments
From: Duy Nguyen @ 2016-09-09 10:54 UTC (permalink / raw)
To: Christian Neukirchen; +Cc: Git Mailing List
In-Reply-To: <87zinmhx68.fsf@juno.home.vuxu.org>
On Tue, Sep 6, 2016 at 4:08 AM, Christian Neukirchen
<chneukirchen@gmail.com> wrote:
> Hi,
>
> I noticed the following suprising behavior:
>
> % git --version
> git version 2.10.0
>
> % git add bar
> % git status -s
> A bar
> M foo
>
> % git commit -p foo
> [stage a hunk]
> ...
> # Explicit paths specified without -i or -o; assuming --only paths...
> # On branch master
> # Changes to be committed:
> # new file: bar
> # modified: foo
> #
>
> So why does it want to commit bar too, when I explicitly wanted to
> commit foo only?
>
> This is not how "git commit files..." works, and the man page says
>
> 3.by listing files as arguments to the commit command, in which
> case the commit will ignore changes staged in the index, and
> instead record the current content of the listed files (which must
> already be known to Git);
>
> I'd expect "git commit -p files..." to work like
> "git add -p files... && git commit files...".
The paths after '-p' could mean two things, either as a filter (e.g.
like in "git add -p") to help save your time going through all changed
files, or as "git commit files...". I think the paths were meant to be
filter when '-p' was added. There's a separate bullet point git-commit
man page, number 5, in about --patch, so that paragraph you quoted is
probably _not_ about --patch. Either way changing its behavior now
might surprise users used to it.
At the least I think we should clarify this in the document. Maybe we
could add --patch-only as well, which commits just what you select in
--patch mode, ignoring anything in existing index.
--
Duy
^ permalink raw reply
* Re: [PATCH 2/3] t0001: work around the bug that reads config file before repo setup
From: Jeff King @ 2016-09-09 11:22 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Michael J Gruber, Max Nordlund
In-Reply-To: <CACsJy8DmbdGODY+qT38OSkaegSpdO7yAt6e67sHB=_BT8Y7=_Q@mail.gmail.com>
On Fri, Sep 09, 2016 at 05:32:21PM +0700, Duy Nguyen wrote:
> On Fri, Sep 9, 2016 at 3:02 AM, Jeff King <peff@peff.net> wrote:
> > On Thu, Sep 08, 2016 at 08:47:18PM +0700, Nguyễn Thái Ngọc Duy wrote:
> >
> >> git-init somehow reads '.git/config' at current directory and sets
> >> log_all_ref_updates based on this file. Because log_all_ref_updates is
> >> not unspecified (-1) any more. It will not be written to the new repo's
> >> config file (see create_default_files() function).
> >>
> >> This will affect our tests in the next patch as we will compare the
> >> config file and expect that core.logallrefupdates is already set to true
> >> by "git init main-worktree".
> >
> > This is a bug for more than worktrees, and is something I'm working on
> > fixing
>
> Great! test_expect_failure it is. But I'll make a separate patch,
> independent from this series though.
If you're curious what the fix looks like, it's in:
https://github.com/peff/git jk/config-repo-setup
The actual fix is in the final patch, but it needed a lot of preparatory
work to avoid breaking various programs that made bad assumptions (and
in the process, I uncovered a ton of other minor bugs).
This is just a preview in case you're interested, for two reasons:
1. I literally _just_ put the finishing touches on it, and it's
extensive and tricky enough that I really should give it one more
proofread.
2. There may be other related fallouts from the bug related to running
"git init /path/to/foo" when "/path/to/foo" already exists (and in
that case we _do_ want to read its config, but not the config from
an existing repository). This may all just work fine, but I need to
think about some tests.
-Peff
^ permalink raw reply
* Re: [PATCH v2] rebase -i: improve advice on bad instruction lines
From: Johannes Schindelin @ 2016-09-09 12:21 UTC (permalink / raw)
To: Ralf Thielow; +Cc: Dennis Kaarsemaker, git, Junio C Hamano
In-Reply-To: <CAN0XMO+diDLXUrxjNGnUi6oUEwkcGavEX8vyGWy2+Wju-N4WJQ@mail.gmail.com>
Hi Ralf,
On Wed, 7 Sep 2016, Ralf Thielow wrote:
> 2016-09-07 11:28 GMT+02:00 Dennis Kaarsemaker <dennis@kaarsemaker.net>:
> > Hi Ralf,
> >
> > There are quite a few patch series in flight these days around
> > interactive rebase. Have you checked for conflicts with those?
> >
>
> Thanks. I did not check against 'pu' when I created this patch but I'm able
> to apply without any conflicts.
Yeah, rewrites in C won't conflict with your patches. If you rebase
interactively, the worst that will happen is that you update code that was
moved into contrib/...
Ciao,
Johannes
^ 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