* Re: git/git-scm.com GH Issue Tracker
From: Duy Nguyen @ 2017-02-06 10:18 UTC (permalink / raw)
To: Samuel Lijin; +Cc: git@vger.kernel.org
In-Reply-To: <CAJZjrdX_8tjMhRac9QQOW8m_S2DprFPV=uZp8mFT+g6bASVd-w@mail.gmail.com>
On Mon, Feb 6, 2017 at 1:15 PM, Samuel Lijin <sxlijin@gmail.com> wrote:
> # Irrelevant but someone should take a look
>
> 693
To save people some time (and since i looked at it anyway), this is
about whether "warning in tree xxx: contains zero-padded file modes:
from fsck should be a warning or error. It is a warning now even
though "git -c transfer.fsckobjects=true clone" treats it as an error.
There are some discussions in the past [1] [2] about this.
There's also a question "And I failed to find in the documentation if
transfer.fsckobjects could be disabled per repository, can you confirm
it's not possible for now ?"
(sorry no answer from me)
[1] http://public-inbox.org/git/%3CCAEBDL5W3DL0v=TusuB7Vg-4bWdAJh5d2Psc1N0Qe+KK3bZH3=Q@mail.gmail.com%3E/
[2] http://public-inbox.org/git/%3C20100326215600.GA10910@spearce.org%3E/
--
Duy
^ permalink raw reply
* Re: gitconfig get out of sync with submodule entries on branch switch
From: Stefan Beller @ 2017-02-06 10:35 UTC (permalink / raw)
To: Benjamin Schindler; +Cc: git@vger.kernel.org
In-Reply-To: <0f14df64-1aa2-e671-9785-4e5e0a076ae6@gmail.com>
Answering the original email, as I feel we're going down the wrong rabbit
hole in the existing thread.
On Mon, Jan 30, 2017 at 8:21 AM, Benjamin Schindler
<beschindler@gmail.com> wrote:
> Hi
>
> Consider the following usecase: I have the master branch where I have a
> submodule A. I create a branch where I rename the submodule to be in the
> directory B. After doing all of this, everything looks good.
> Now, I switch back to master. The first oddity is, that it fails to remove
> the folder B because there are still files in there:
>
> bschindler@metis ~/Projects/submodule_test (testbranch) $ git checkout
> master
> warning: unable to rmdir other_submodule: Directory not empty
> Switched to branch 'master'
checkout currently doesn't support submodules, so it should neither
try to delete B nor try to repopulate A when switching back to master.
checkout ought to not even touch the existing submodule B.
>
> Git submodule deinit on B fails because the submodule is not known to git
> anymore (after all, the folder B exists only in the other branch). I can
> easily just remove the folder B from disk and initialize the submodule A
> again, so all seems good.
by initializing you mean populating(?), i.e.
git submodule update
would work without the --init flag or preceding "git submodule init A".
That ought to not redownload A, but just put files back in the working tree
from the submodule git directory inside the superprojects git dir.
>
> However, what is not good is that the submodule b is still known in
> .git/config.
Oh, I see. You did not just rename the path, but also the name
in the .gitmodules?
> This is in particular a problem for us, because I know a number
> of tools which use git config to retrieve the submodule list. Is it
> therefore a bug that upon branch switch, the submodule gets deregistered,
> but its entry in .git/config remains?
The config remains as it indicates that you express(ed) interest in
submodule A, such that when switching branches
master->renamedToB->master
then we'd still care about A. As for the tools, I'd rather see them use
git submodule status/summary
instead of directly looking at the config, because the config may
change in the future.
>
> thanks a lot
> Benjamin Schindler
>
> P.s. I did not subscribe to the mailing list, please add me at least do CC.
> Thanks
^ permalink raw reply
* Re: gitconfig get out of sync with submodule entries on branch switch
From: Benjamin Schindler @ 2017-02-06 12:17 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kaZUOO4qusCDF9=VJ-6QPjAvc5eSaazjWWEocRMHuTSug@mail.gmail.com>
On 06.02.2017 11:35, Stefan Beller wrote:
> Answering the original email, as I feel we're going down the wrong rabbit
> hole in the existing thread.
>
> On Mon, Jan 30, 2017 at 8:21 AM, Benjamin Schindler
> <beschindler@gmail.com> wrote:
>> Hi
>>
>> Consider the following usecase: I have the master branch where I have a
>> submodule A. I create a branch where I rename the submodule to be in the
>> directory B. After doing all of this, everything looks good.
>> Now, I switch back to master. The first oddity is, that it fails to remove
>> the folder B because there are still files in there:
>>
>> bschindler@metis ~/Projects/submodule_test (testbranch) $ git checkout
>> master
>> warning: unable to rmdir other_submodule: Directory not empty
>> Switched to branch 'master'
>
> checkout currently doesn't support submodules, so it should neither
> try to delete B nor try to repopulate A when switching back to master.
> checkout ought to not even touch the existing submodule B.
Well, it tried to remove the folder (the rmdir warning) but it failed so
in some sense you are right. Is there a technical reason for this
default though? Here, I frequently have to point out to people that they
need to initialize/update the submodule on e.g. clone.
>
>>
>> Git submodule deinit on B fails because the submodule is not known to git
>> anymore (after all, the folder B exists only in the other branch). I can
>> easily just remove the folder B from disk and initialize the submodule A
>> again, so all seems good.
>
> by initializing you mean populating(?), i.e.
>
> git submodule update
>
> would work without the --init flag or preceding "git submodule init A".
> That ought to not redownload A, but just put files back in the working tree
> from the submodule git directory inside the superprojects git dir.
>
>>
>> However, what is not good is that the submodule b is still known in
>> .git/config.
>
> Oh, I see. You did not just rename the path, but also the name
> in the .gitmodules?
I wasn't even aware that the submodule name was something different from
the path because the name is by default set to be the path to it. So
yes, I didn't just relocate it, it had a different name.
>
>> This is in particular a problem for us, because I know a number
>> of tools which use git config to retrieve the submodule list. Is it
>> therefore a bug that upon branch switch, the submodule gets deregistered,
>> but its entry in .git/config remains?
>
> The config remains as it indicates that you express(ed) interest in
> submodule A, such that when switching branches
>
> master->renamedToB->master
>
> then we'd still care about A. As for the tools, I'd rather see them use
>
> git submodule status/summary
>
> instead of directly looking at the config, because the config may
> change in the future.
That was my feeling but its good to know to have more solid reasons why
that would be.
Cheers
Benjamin
>
>>
>> thanks a lot
>> Benjamin Schindler
>>
>> P.s. I did not subscribe to the mailing list, please add me at least do CC.
>> Thanks
^ permalink raw reply
* A little help understanding output from git blame --reverse
From: Edmundo Carmona Antoranz @ 2017-02-06 12:38 UTC (permalink / raw)
To: Git List
Hi!
While doing some research developing difflame I found some output from
git blame --reverse that I can't quite understand. Perhaps another set
of eyeballs could help me.
I'm "difflaming" HEAD~100 (02db2d0421b97fcb6211) and HEAD
(066fb0494e6398eb). Specifically file abspath.c.
If we diff (as in plain old git diff) HEAD~100..HEAD we can see that
line 63 (from HEAD~100 revision) was deleted between HEAD~100 and
HEAD:
@@ -58,86 +95,136 @@ blah blah
goto error_out;
}
- strbuf_reset(&sb);
- strbuf_addstr(&sb, path);
-
- while (depth--) {
- if (!is_directory(sb.buf)) {
So, if I do a "reverse" blame operation on the file, I would expect to
see the last revision where that line was _present_ on the file:
c5f3cba126 61) strbuf_reset(&sb);
c5f3cba126 62) strbuf_addstr(&sb, path);
066fb0494e 63)
c5f3cba126 64) while (depth--) {
c5f3cba126 65) if (!is_directory(sb.buf)) {
line 63 shows up as if it had been last present on the file on
revision 066fb0494e, which is HEAD, which kind of doesn't make a lot
of sense to me because given that the line is not present on the file
on HEAD (as we can guess from diff output) it means it was "forcefully
present" on some previous revision (and that's what I would expect to
see reported on blame --reverse output).
What am I missing? Thanks in advance.
^ permalink raw reply
* Re: A little help understanding output from git blame --reverse
From: Edmundo Carmona Antoranz @ 2017-02-06 12:40 UTC (permalink / raw)
To: Git List
In-Reply-To: <CAOc6etZ7iuPKRQkYSZDrDRW0hxbu1aYMRuzB1iXAPv+EEnXJEg@mail.gmail.com>
On Mon, Feb 6, 2017 at 6:38 AM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> I'm "difflaming" HEAD~100 (02db2d0421b97fcb6211) and HEAD
> (066fb0494e6398eb). Specifically file abspath.c.
I just noticed that I'm standing on a private branch. Replace HEAD for
"4e59582ff" when doing your analysis. You should get the same results.
^ permalink raw reply
* [PATCH] worktree: fix option descriptions for `prune`
From: Patrick Steinhardt @ 2017-02-06 13:13 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Nguyễn Thái Ngọc Duy, ps
The `verbose` and `expire` options of the `git worktree prune`
subcommand have wrong descriptions in that they pretend to relate to
objects. But as the git-worktree(1) correctly states, these options have
nothing to do with objects but only with worktrees. Fix the description
accordingly.
Signed-off-by: Patrick Steinhardt <patrick.steinhardt@elego.de>
---
builtin/worktree.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9a97e37a3..831fe058a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -125,9 +125,9 @@ static int prune(int ac, const char **av, const char *prefix)
{
struct option options[] = {
OPT__DRY_RUN(&show_only, N_("do not remove, show only")),
- OPT__VERBOSE(&verbose, N_("report pruned objects")),
+ OPT__VERBOSE(&verbose, N_("report pruned working trees")),
OPT_EXPIRY_DATE(0, "expire", &expire,
- N_("expire objects older than <time>")),
+ N_("expire working trees older than <time>")),
OPT_END()
};
--
2.11.1
^ permalink raw reply related
* [PATCH v3] tag: generate useful reflog message
From: cornelius.weig @ 2017-02-06 13:58 UTC (permalink / raw)
To: git; +Cc: Cornelius Weig, karthik.188, peff, gitster
In-Reply-To: <xmqqo9yg43uo.fsf@gitster.mtv.corp.google.com>
From: Cornelius Weig <cornelius.weig@tngtech.com>
When tags are created with `--create-reflog` or with the option
`core.logAllRefUpdates` set to 'always', a reflog is created for them.
So far, the description of reflog entries for tags was empty, making the
reflog hard to understand. For example:
6e3a7b3 refs/tags/test@{0}:
Now, a reflog message is generated when creating a tag, following the
pattern "tag: tagging <short-sha1> (<description>)". If
GIT_REFLOG_ACTION is set, the message becomes "$GIT_REFLOG_ACTION
(<description>)" instead. If the tag references a commit object, the
description is set to the subject line of the commit, followed by its
commit date. For example:
6e3a7b3 refs/tags/test@{0}: tag: tagging 6e3a7b3398 (Git 2.12-rc0, 2017-02-03)
If the tag points to a tree/blob/tag objects, the following static
strings are taken as description:
- "tree object"
- "blob object"
- "other tag object"
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
builtin/tag.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
t/t7004-tag.sh | 16 +++++++++++++++-
2 files changed, 68 insertions(+), 2 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index e40c4a9..638b68e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -302,6 +302,54 @@ static void create_tag(const unsigned char *object, const char *tag,
}
}
+static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
+{
+ enum object_type type;
+ struct commit *c;
+ char *buf;
+ unsigned long size;
+ int subject_len = 0;
+ const char *subject_start;
+
+ char *rla = getenv("GIT_REFLOG_ACTION");
+ if (rla) {
+ strbuf_addstr(sb, rla);
+ } else {
+ strbuf_addstr(sb, _("tag: tagging "));
+ strbuf_add_unique_abbrev(sb, sha1, DEFAULT_ABBREV);
+ }
+
+ strbuf_addstr(sb, " (");
+ type = sha1_object_info(sha1, NULL);
+ switch (type) {
+ default:
+ strbuf_addstr(sb, _("internal object"));
+ break;
+ case OBJ_COMMIT:
+ c = lookup_commit_reference(sha1);
+ buf = read_sha1_file(sha1, &type, &size);
+ if (!c || !buf) {
+ die(_("commit object %s could not be read"),
+ sha1_to_hex(sha1));
+ }
+ subject_len = find_commit_subject(buf, &subject_start);
+ strbuf_insert(sb, sb->len, subject_start, subject_len);
+ strbuf_addf(sb, ", %s", show_date(c->date, 0, DATE_MODE(SHORT)));
+ free(buf);
+ break;
+ case OBJ_TREE:
+ strbuf_addstr(sb, _("tree object"));
+ break;
+ case OBJ_BLOB:
+ strbuf_addstr(sb, _("blob object"));
+ break;
+ case OBJ_TAG:
+ strbuf_addstr(sb, _("other tag object"));
+ break;
+ }
+ strbuf_addch(sb, ')');
+}
+
struct msg_arg {
int given;
struct strbuf buf;
@@ -335,6 +383,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
{
struct strbuf buf = STRBUF_INIT;
struct strbuf ref = STRBUF_INIT;
+ struct strbuf reflog_msg = STRBUF_INIT;
unsigned char object[20], prev[20];
const char *object_ref, *tag;
struct create_tag_options opt;
@@ -494,6 +543,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
else
die(_("Invalid cleanup mode %s"), cleanup_arg);
+ create_reflog_msg(object, &reflog_msg);
+
if (create_tag_object) {
if (force_sign_annotate && !annotate)
opt.sign = 1;
@@ -504,7 +555,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (!transaction ||
ref_transaction_update(transaction, ref.buf, object, prev,
create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
- NULL, &err) ||
+ reflog_msg.buf, &err) ||
ref_transaction_commit(transaction, &err))
die("%s", err.buf);
ref_transaction_free(transaction);
@@ -514,5 +565,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
strbuf_release(&err);
strbuf_release(&buf);
strbuf_release(&ref);
+ strbuf_release(&reflog_msg);
return 0;
}
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 072e6c6..3c4cb58 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -80,10 +80,24 @@ test_expect_success 'creating a tag using default HEAD should succeed' '
test_must_fail git reflog exists refs/tags/mytag
'
+git log -1 > expected \
+ --format="format:tag: tagging %h (%s, %cd)%n" --date=format:%F
test_expect_success 'creating a tag with --create-reflog should create reflog' '
test_when_finished "git tag -d tag_with_reflog" &&
git tag --create-reflog tag_with_reflog &&
- git reflog exists refs/tags/tag_with_reflog
+ git reflog exists refs/tags/tag_with_reflog &&
+ sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+ test_cmp expected actual
+'
+
+git log -1 > expected \
+ --format="format:tag: tagging %h (%s, %cd)%n" --date=format:%F
+test_expect_success 'annotated tag with --create-reflog has correct message' '
+ test_when_finished "git tag -d tag_with_reflog" &&
+ git tag -m "annotated tag" --create-reflog tag_with_reflog &&
+ git reflog exists refs/tags/tag_with_reflog &&
+ sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+ test_cmp expected actual
'
test_expect_success '--create-reflog does not create reflog on failure' '
--
2.10.2
^ permalink raw reply related
* Re: [PATCH] p5302: create repositories for index-pack results explicitly
From: Jeff King @ 2017-02-06 14:41 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Git List
In-Reply-To: <251bdc20-19a7-9a6c-9f5a-c7e661810c70@web.de>
On Sun, Feb 05, 2017 at 12:43:29PM +0100, René Scharfe wrote:
> Before 7176a314 (index-pack: complain when --stdin is used outside of a
> repo) index-pack silently created a non-existing target directory; now
> the command refuses to work unless it's used against a valid repository.
> That causes p5302 to fail, which relies on the former behavior. Fix it
> by setting up the destinations for its performance tests using git init.
Ah, right. Thanks for catching this.
I think p5302 was wrong to rely on the old behavior, and your patch is
the right fix.
-Peff
^ permalink raw reply
* Re: [PATCH v3 5/5] stash: teach 'push' (and 'create') to honor pathspec
From: Jeff King @ 2017-02-06 15:20 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Gummerer, git, Stephan Beyer, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <xmqqmvdz3ied.fsf@gitster.mtv.corp.google.com>
On Sun, Feb 05, 2017 at 11:09:14PM -0800, Junio C Hamano wrote:
> > @@ -99,6 +104,10 @@ create_stash () {
> > if test -z "$new_style"
> > then
> > stash_msg="$*"
> > + while test $# != 0
> > + do
> > + shift
> > + done
> > fi
>
> The intent is correct, but I would probaly empty the "$@" not with
> an iteration of shifts but with a single
>
> set x && shift
>
> ;-)
Perhaps it is not portable, but I think "set --" does the same thing and
is perhaps less obscure.
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/5] Documentation/stash: remove mention of git reset --hard
From: Jeff King @ 2017-02-06 15:22 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170205202642.14216-2-t.gummerer@gmail.com>
On Sun, Feb 05, 2017 at 08:26:38PM +0000, Thomas Gummerer wrote:
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 2e9cef06e6..2e9e344cd7 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -47,8 +47,9 @@ OPTIONS
>
> save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
>
> - Save your local modifications to a new 'stash', and run `git reset
> - --hard` to revert them. The <message> part is optional and gives
> + Save your local modifications to a new 'stash' and roll them
> + back to HEAD (in the working tree and in the index).
> + The <message> part is optional and gives
Nice. I think what you ended up with here is concise and accurate.
-Peff
^ permalink raw reply
* Cross-referencing the Git mailing list archive with their corresponding commits in `pu`
From: Johannes Schindelin @ 2017-02-06 15:34 UTC (permalink / raw)
To: Josh Triplett; +Cc: git
Hi Josh,
as discussed at the GitMerge, I am trying to come up with tooling that
will allow for substantially less tedious navigation between the local
repository, the mailing list, and what ends up in the `pu` branch.
That tooling would *still* not help lowering the barrier of entry for
contributing to Git by a lot, as it would *still* not address the problem
that mails sent from the most prevalent desktop mail client, as well as
mails sent from the most prevalent web mail client, are simply and
unceremoniously dropped. (This problem was acknowledged by quite a few
nods even at the Contributors' Summit...) But still, we decided to start
*somewhere* and this tooling is what we agreed on.
It is quite a bit harder going than I would like: as we have figured out,
the Subject: line is not a good way to link the commits with the original
mails containing the patches, as commit messages are modified before being
pushed often enough to make this a fragile matching.
So I thought maybe the From: line (from the body, if available, otherwise
from the header) in conjunction with the "Date:" header would work. But a
preliminary study shows that there are 336 From: + Date: combinations in
the Git mailing list archive that are not unique. 71 of these are shared
by three or more mails, even, and 9 are shared by more than 10 mails,
respectively. This is bad!
Unsurprisingly, the top 10 of these cases were obviously caused by the
builtin `git am` bug where it would not reset the author date properly.
Surprisingly, though, there were a few cases from 2005, too.
I had a quick look to find out what was the culprit (looking at the
17-strong patch series "Documentation fixes in response to my previous
listing" by Nikolai Weibull, but I am at a loss there: the mail claims to
be sent by git-send-email and the patches appear to be generated by
git-format-patch as of v0.99.9l, neither of which had a Date:-related bug
back in that time frame. My best guess is that the patches were mishandled
by a tool similar to rebase -i (which entered Git only at v1.5.3).
For details, see:
http://public-inbox.org/git/11340844841342-git-send-email-mailing-lists.git@rawuncut.elitemail.org/
(this is also an example where public-inbox' thread detection went utterly
wrong, including way too many mails in the "thread")
There was even a case of duplicated Date: headers in 2012. Now, this case
is very curious, as there have been 7 mails with identical Date: header,
but it was not a 6-strong patch series. Instead, it was a 4-strong patch
series that needed three iterations before it was accepted, and the
identical Date: header appears only in v2's patches (*not* in its cover
letter) and it *disappeared* in v3's 4/4, where it was set *back* by a
week (to the Date: it had in v1).
For details, see
http://public-inbox.org/git/cover.1354693001.git.Sebastian.Leske@sleske.name/
and
http://public-inbox.org/git/cover.1354324110.git.Sebastian.Leske@sleske.name/
and
http://public-inbox.org/git/b115a546fa783b4121d118bb8fdb9270443f90fa.1353691892.git.Sebastian.Leske@sleske.name/
This last example also demonstrates a very curious test case for a
different difficulty in trying to reconstruct lost correspondences: the
patch series was applied *twice*, independently of each other. First, on
the day v3 was submitted, it was applied on top of v1.8.1-rc0 (as commits
ee26a6e2b8..dd465ce66f), although it was not merged until v1.8.1-rc3. 22
days later, it was reapplied on top of maint so it could enter v1.8.0.3
(back then, Git still had "patchlevel" versions): c2999adcd5..008c208c2c.
As you can see, there is a many-to-many relationship here, even if you do
leave the *original* branch out of the picture entirely.
Will keep you posted,
Dscho
P.S.: I used public-inbox.org links instead of commit references to the
Git repository containing the mailing list archive, because the format of
said Git repository is so unfavorable that it was determined very quickly
in a discussion between Patrick Reynolds (GitHub) and myself that it would
put totally undue burden on GitHub to mirror it there (compare also Carlos
Nieto's talk at GitMerge titled "Top Ten Worst Repositories to host on
GitHub").
^ permalink raw reply
* Re: [PATCH v3 2/5] stash: introduce push verb
From: Jeff King @ 2017-02-06 15:46 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170205202642.14216-3-t.gummerer@gmail.com>
On Sun, Feb 05, 2017 at 08:26:39PM +0000, Thomas Gummerer wrote:
> + -m|--message)
> + shift
> + stash_msg=${1?"-m needs an argument"}
> + ;;
I think this is our first use of the "?" parameter expansion magic. It
_is_ in POSIX, so it may be fine. We may get complaints from people on
weird shell variants, though. If that's the only reason to avoid it, I'd
be inclined to try it and see, as it is much shorter.
OTOH, most of the other usage errors call usage(), and this one doesn't.
Nor is the error message translatable. Perhaps we should stick to the
longer form (and add a helper function if necessary to reduce the
boilerplate).
> +save_stash () {
> + push_options=
> + while test $# != 0
> + do
> + case "$1" in
> + --help)
> + show_help
> + ;;
> + --)
> + shift
> + break
> + ;;
> + -*)
> + # pass all options through to push_stash
> + push_options="$push_options $1"
> + ;;
> + *)
> + break
> + ;;
> + esac
> + shift
> + done
I suspect you could just let "--help" get handled in the pass-through
case (it generally takes precedence over errors found in other options,
but I do not see any other parsing errors that could be found by this
loop). It is not too bad to keep it, though (the important thing is that
we're not duplicating all of the push_stash options here).
> + if test -z "$stash_msg"
> + then
> + push_stash $push_options
> + else
> + push_stash $push_options -m "$stash_msg"
> + fi
Hmm. So $push_options is subject to word-splitting here. That's
necessary to split the options back apart. It does the wrong thing if
any of the options had spaces in them. But I don't think there are any
valid options which do so, and "save" would presumably not grow any new
options (they would go straight to "push").
So there is a detectable behavior change:
[before]
$ git stash "--bogus option"
error: unknown option for 'stash save': --bogus option
To provide a message, use git stash save -- '--bogus option'
[etc...]
[after]
$ git stash "--bogus option"
error: unknown option for 'stash save': --bogus
To provide a message, use git stash save -- '--bogus'
but it's probably an acceptable casualty (the "right" way would be to
shell-quote everything you stuff into $push_options and then eval the
result when you invoke push_stash).
Likewise, it's usually a mistake to just stick a new option (like "-m")
after a list of unknown options. But it's OK here because we know we
removed any "--" or non-option arguments.
-Peff
^ permalink raw reply
* Re: [PATCH v3 3/5] stash: add test for the create command line arguments
From: Jeff King @ 2017-02-06 15:50 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170205202642.14216-4-t.gummerer@gmail.com>
On Sun, Feb 05, 2017 at 08:26:40PM +0000, Thomas Gummerer wrote:
> +test_expect_success 'create stores correct message' '
> + >foo &&
> + git add foo &&
> + STASH_ID=$(git stash create "create test message") &&
> + echo "On master: create test message" >expect &&
> + git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
> + test_cmp expect actual
> +'
This misses failure exit codes from "git show" because it's on the left
side of a pipe. Perhaps "git show -s" or "git log -1" would get you the
same output without needing "head" (and saving a process and the time
spent generating the diff, as a bonus).
Ditto in the other tests from this patch and later ones.
-Peff
^ permalink raw reply
* Re: [PATCH v3 4/5] stash: introduce new format create
From: Jeff King @ 2017-02-06 15:56 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170205202642.14216-5-t.gummerer@gmail.com>
On Sun, Feb 05, 2017 at 08:26:41PM +0000, Thomas Gummerer wrote:
> git stash create currently supports a positional argument for adding a
> message. This is not quite in line with how git commands usually take
> comments (using a -m flag).
>
> Add a new syntax for adding a message to git stash create using a -m
> flag. This is with the goal of deprecating the old style git stash
> create with positional arguments.
>
> This also adds a -u argument, for untracked files. This is already used
> internally as another positional argument, but can now be used from the
> command line.
How do we tell the difference between new-style invocations, and
old-style ones that look new-style? IOW, I think:
git stash create -m works
currently treats "-m works" as the full message, and it would now become
just "works".
That may be an acceptable loss for the benefit we are getting. The
alternative is to make yet another verb for create, as we did with
save/push). I have a feeling that hardly anybody uses "create", though,
and it's mostly an implementation detail. So given the obscure nature,
maybe it's an acceptable level of regression. I dunno.
But either way, it should probably be in the commit message in case
somebody does have to track it down later.
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/5] stash: support pathspec argument
From: Jeff King @ 2017-02-06 16:14 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170205202642.14216-1-t.gummerer@gmail.com>
On Sun, Feb 05, 2017 at 08:26:37PM +0000, Thomas Gummerer wrote:
> Thanks Junio for the review in the previous round.
>
> Changes since v2:
>
> - $IFS should now be supported by using "$@" everywhere instead of using
> a $files variable.
> - Added a new patch showing the old behaviour of git stash create is
> preserved.
> - Rephrased the documentation
> - Simplified the option parsing in save_stash, by leaving the
> actual parsing to push_stash instead.
Overall, I like the direction this is heading. I raised a few issues,
the most major of which is whether we want to allow the minor regression
in "git stash create -m foo".
This also makes "git stash push -p <pathspec...>" work, which is good. I
don't think "git stash -p <pathspec...>" does, though. I _think_ it
would be trivial to do on top, since we already consider that case an
error. That's somewhat outside the scope of your series, so I won't
complain (too much :) ) if you don't dig into it, but it might just be
trivial on top.
A few other random bits I noticed while playing with the new code:
$ git init
$ echo content >file && git add file && git commit -m file
$ echo change >file
$ git stash push -p not-file
No changes.
No changes selected
Probably only one of those is necessary. :)
Let's keep trying a few things:
$ git stash push not-file
Saved working directory and index state WIP on master: 5d5f951 file
Unstaged changes after reset:
M file
M file
The unstaged change is mentioned twice, which is weird. But weirder
still is that we created a stash at all, as it's empty. Why didn't we
hit the "no changes selected" path?
And one more:
$ echo foo >untracked
$ git stash push untracked
Saved working directory and index state WIP on master: 5d5f951 file
Unstaged changes after reset:
M file
M file
Removing untracked
We removed the untracked file, even though it wasn't actually stashed! I
thought at first this was because it was mentioned as a pathspec, but it
seems to happen even with a different name:
$ echo foo >untracked
$ git stash push does-not-exist
...
Removing untracked
That seems dangerous.
-Peff
^ permalink raw reply
* Re: git-scm.com status report
From: Jeff King @ 2017-02-06 16:24 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git List
In-Reply-To: <CAFZEwPPX4LngKrOHxgEp4aMGKOs0w4LHUBKumtmeRJSZ2_iV_Q@mail.gmail.com>
On Mon, Feb 06, 2017 at 01:41:04AM +0530, Pranit Bauva wrote:
> On Thu, Feb 2, 2017 at 8:03 AM, Jeff King <peff@peff.net> wrote:
> > ## What's on the site
> >
> > We have the domains git-scm.com and git-scm.org (the latter we've had
> > for a while). They both point to the same website, which has general
> > information about Git, including:
>
> Since we have an "official" control over the website, shouldn't we be
> using the .org domain more because we are more of an organization?
> What I mean is that in many places, we have referred to git-scm.com,
> which was perfectly fine because it was done by github which is a
> company but now I think it would be more appropriate to use
> git-scm.org domain. We can forward all .com requests to .org and try
> to move all reference we know about, to .org. What do you all think?
I don't have a preference myself. I know a lot of non-commercial groups
(which I think the Git project is) try to prefer ".org" to signal that.
Switching it around would require some DNS changes. I think ".org" goes
to a server the DNS provider (Gandi) runs which issues an HTTP 301 to
".com". So we'd want to reverse that, or possibly just treat them both
as equals. That shouldn't be too hard, and will have to be done via
Conservancy.
I don't know what it would mean in terms of search-engine optimization.
I know Google tries to detect duplicate names for sites and treat one as
canonical. And that's going to be ".com" now, based on the existing
redirect and on the fact that most people will have linked to .com.
I'm not sure what disadvantages there are to switching now, or if there
are things we should be doing to tell search engines (I seem to recall
Google's Webmaster tools have options to say "this is the canonical
name"). This is pretty far outside my area of expertise, so it may not
even be something to care about at all. Just things to consider (and
hopefully more clueful people than I can comment on it).
-Peff
^ permalink raw reply
* Re: BUG: "git checkout -q -b foo origin/foo" is not quiet
From: Kevin Layer @ 2017-02-06 16:38 UTC (permalink / raw)
To: Cornelius Weig; +Cc: git
In-Reply-To: <3bd29833-8fb2-5097-f735-6a3f61e678bf@tngtech.com>
Yeah, my bad. I was confused. Sorry for the noise.
On Fri, Feb 3, 2017 at 4:55 PM, Cornelius Weig
<cornelius.weig@tngtech.com> wrote:
> On 02/03/2017 10:36 PM, Kevin Layer wrote:
>> Note that git version 1.8.3.1 is quiet and does not print the
>> "tracking remote" message.
>
> So what you are saying is, that git v1.8.3.1 *is* quiet, but v1.7.1 is
> not? Sounds like a fixed bug to me.
>
> I also checked with the latest version and it did not print anything
> when used with -q.
>
>
> You seem to urgently need quiet branch creation. Have you thought about
> dumping unwanted output in the shell? E.g. in bash
>
> $ git whatever >&/dev/null
^ permalink raw reply
* Re: feature request: add -q to "git branch"
From: Kevin Layer @ 2017-02-06 16:39 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git List
In-Reply-To: <CAFZEwPPyAxeU2i-OL62O749GaTdL7H19jbbAj8R6fipVnUjt=Q@mail.gmail.com>
I think I got my git versions (old and new) mixed up. Sorry for the noise.
On Sat, Feb 4, 2017 at 1:17 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Hey Kevin,
>
> Sorry for the previous message.
>
> On Sun, Feb 5, 2017 at 2:47 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Hey Kevin,
>>
>> On Fri, Feb 3, 2017 at 11:59 PM, Kevin Layer <layer@known.net> wrote:
>>> It should be possible to quietly create a branch.
>
> I think `git branch` is already quiet. Are you seeing something else?
>
> Regards,
> Pranit Bauva
^ permalink raw reply
* Re: [PATCH] tag: generate useful reflog message
From: Cornelius Weig @ 2017-02-06 16:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, bitte.keine.werbung.einwerfen, karthik.188, peff
In-Reply-To: <xmqqo9yg43uo.fsf@gitster.mtv.corp.google.com>
On 02/06/2017 12:25 AM, Junio C Hamano wrote:
> cornelius.weig@tngtech.com writes
> For a tag, I would imagine something like "tag: tagged 4e59582ff7
> ("Seventh batch for 2.12", 2017-01-23)" would be more appropriate.
Yes, I agree that this is much clearer. The revised version v3
implements this behavior.
>> Notes:
>> While playing around with tag reflogs I also found a bug that was present
>> before this patch. It manifests itself when the sha1-ref in the reflog does not
>> point to a commit object but something else.
>
> I think the fix would involve first ripping out the "reflog walking"
> code that was bolted on and stop allowing it to inject the entries
> taken from the reflog into the "walk the commit DAG" machinery.
> Then "reflog walking" code needs to be taught to have its own "now
> we got a single object to show, show it (using the helper functions
> to show a single object that is already used by 'git show')" code,
> instead of piggy-backing on the output codepath used by "log" and
> "rev-list".
I'll start investigating how that could be done. My first glance tells
me that it won't be easy. Especially because I'm not yet familiar with
the git code.
Thanks for your advice!
^ permalink raw reply
* Re: [PATCH] difftool: fix bug when printing usage
From: Johannes Schindelin @ 2017-02-06 16:58 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, Git ML, Denton Liu
In-Reply-To: <20170205212338.17667-1-davvid@gmail.com>
Hi David,
On Sun, 5 Feb 2017, David Aguilar wrote:
> "git difftool -h" reports an error:
>
> fatal: BUG: setup_git_env called without repository
>
> Defer repository setup so that the help option processing happens before
> the repository is initialized.
>
> Add tests to ensure that the basic usage works inside and outside of a
> repository.
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> This bug exists in both "master" and "next".
> This patch has been tested on both branches.
Thanks for noticing and for patching!
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index b5e85ab079..d13350ce83 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -647,10 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
> OPT_END()
> };
>
> - /* NEEDSWORK: once we no longer spawn anything, remove this */
> - setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
> - setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
> -
> git_config(difftool_config, NULL);
> symlinks = has_symlinks;
>
> @@ -661,6 +657,10 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
> if (tool_help)
> return print_tool_help();
>
> + /* NEEDSWORK: once we no longer spawn anything, remove this */
> + setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
> + setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
> +
> if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
> setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
> else if (difftool_cmd) {
Aaargh. You know, when you are used to code review systems where you can
easily increase the context of the diff hunks, static patches are...
tedious to review.
For the record: the context between those two hunks is:
argc = parse_options(argc, argv, prefix, builtin_difftool_options,
builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |
PARSE_OPT_KEEP_DASHDASH);
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index aa0ef02597..21e2ac4ad6 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -23,6 +23,19 @@ prompt_given ()
> test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
> }
>
> +test_expect_success 'basic usage requires no repo' '
> + lines=$(git difftool -h | grep ^usage: | wc -l) &&
> + test "$lines" -eq 1 &&
It may be easier to debug future breakages if you wrote
git difftool -h | grep ^usage: >output &&
test_line_count = 1 output &&
or even better (changing the semantics now):
test_expect_code 129 git difftool -h >output &&
grep ^usage: output &&
> + # create a ceiling directory to prevent Git from finding a repo
> + mkdir -p not/repo &&
> + ceiling="$PWD/not" &&
> + lines=$(cd not/repo &&
> + GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
> + grep ^usage: | wc -l) &&
> + test "$lines" -eq 1 &&
Likewise, this would become
GIT_CEILING_DIRECTORIES="$PWD/not" \
test_expect_code 129 git -C not/repo difftool -h >output &&
grep ^usage: output
> + rmdir -p not/repo
I am not sure how portable `rmdir -p` is. Why not use
test_when_finished rm -r not
instead?
But those are all really minor bike-sheddings, and I promised myself that
I would avoid those (as I find them rather pointless) unless there is at
least one real benefit. In this case, I think the result becomes more
readable:
-- snip --
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 21e2ac4ad6d..7d7cb63d61e 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -24,16 +24,14 @@ prompt_given ()
}
test_expect_success 'basic usage requires no repo' '
- lines=$(git difftool -h | grep ^usage: | wc -l) &&
- test "$lines" -eq 1 &&
- # create a ceiling directory to prevent Git from finding a repo
- mkdir -p not/repo &&
- ceiling="$PWD/not" &&
- lines=$(cd not/repo &&
- GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
- grep ^usage: | wc -l) &&
- test "$lines" -eq 1 &&
- rmdir -p not/repo
+ test_expect_code 129 git difftool -h >output &&
+ grep ^usage: output &&
+ # create a ceiling directory to prevent Git from finding a repo
+ mkdir -p not/repo &&
+ test_when_finished rm -r not &&
+ GIT_CEILING_DIRECTORIES="$PWD/not" \
+ test_expect_code 129 git -C not/repo difftool -h >output &&
+ grep ^usage: output
'
# Create a file on master and change it on branch
-- snap --
More importantly: When I read $PWD all kinds of alarm bells go off in my
head, as I immediately think of all the issues we have on Windows due to
Git's regression test using POSIX paths all over the place.
In this particular instance, it *does* work, magically, because the POSIX
path $PWD is automatically converted (by the MSYS2 runtime) into a Windows
path when git.exe is called.
Read: I actually tested this, in line with my resolution to avoid to
review merely patches (which would likely miss serious bugs) and instead
try to do full code reviews, including testing. That means that I will
review less, but better.
Insofar as I am the author of the builtin difftool:
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Thanks,
Dscho
^ permalink raw reply related
* Re: subtree merging fails
From: Johannes Schindelin @ 2017-02-06 17:08 UTC (permalink / raw)
To: Stavros Liaskos; +Cc: git
In-Reply-To: <CAEXhnEDW_s5vGKmLA8ie63FivFYwtCASyaD_Sowjj3e5U9wp_Q@mail.gmail.com>
Hi Stavros,
On Mon, 6 Feb 2017, Stavros Liaskos wrote:
> Please check this issue for a description of the bug:
> https://github.com/git/git-scm.com/issues/896#issuecomment-277587626
Just an observation from my side: if I see a request for help on this
mailing list where the sender merely pastes a link and does not bother to
condense and summarize the information in the linked page, to help
potential helpers, I am highly unlikely to even click on that link, let
alone to try to help.
Maybe you would want to spend as much effort on a request for help as you
would like to ask people to spend on a reply?
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
From: Marc Branchaud @ 2017-02-06 17:13 UTC (permalink / raw)
To: Michael Haggerty, Paul Mackerras; +Cc: git
In-Reply-To: <cover.1482164633.git.mhagger@alum.mit.edu>
On 2016-12-19 11:44 AM, Michael Haggerty wrote:
> This patch series changes a bunch of details about how remote-tracking
> references are rendered in the commit list of gitk:
I don't see this series in git v2.12.0-rc0, nor in Paul's gitk repo.
I hope this is an oversight, and not that the series got dropped for
some reason.
M.
> * Omit the "remote/" prefix on normal remote-tracking references. They
> are already distinguished via their two-tone rendering and (usually)
> longer names, and this change saves a lot of visual clutter and
> horizontal space.
>
> * Render remote-tracking references that have more than the usual
> three slashes like
>
> origin/foo/bar
> ^^^^^^^
>
> rather than
>
> origin/foo/bar (formerly remotes/origin/foo/bar)
> ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
>
> , where the indicated part is the prefix that is rendered in a
> different color. Usually, such a reference represents a remote
> branch that contains a slash in its name, so the new split more
> accurately portrays the separation between remote name and remote
> branch name.
>
> * Introduce a separate constant to specify the background color used
> for the branch name part of remote-tracking references, to allow it
> to differ from the color used for local branches (which by default
> is bright green).
>
> * Change the default background colors for remote-tracking branches to
> light brown and brown (formerly they were pale orange and bright
> green).
>
> I understand that the colors of pixels on computer screens is an even
> more emotional topic that that of bikesheds, so I implemented the last
> change as a separate commit, the last one in the series. Feel free to
> drop it if you don't want the default color change.
>
> Along the way, I did a bunch of refactoring in the area to make these
> changes easier, and introduced a constant for the background color of
> "other" references so that it can also be adjusted by users.
>
> (Unfortunately, these colors can only be adjusted by editing the
> configuration file. Someday it would be nice to allow them to be
> configured via the preferences dialog.)
>
> It's been a while since I've written any Tcl code, so I apologize in
> advance for any howlers :-)
>
> This branch applies against the `master` branch in
> git://ozlabs.org/~paulus/gitk.
>
> Michael
>
> Michael Haggerty (13):
> gitk: when processing tag labels, don't use `marks` as scratch space
> gitk: keep track of tag types in a separate `types` array
> gitk: use a type "tags" to indicate abbreviated tags
> gitk: use a type "mainhead" to indicate the main HEAD branch
> gitk: fill in `wvals` as the tags are first processed
> gitk: simplify regexp
> gitk: extract a method `remotereftext`
> gitk: only change the color of the "remote" part of remote refs
> gitk: shorten labels displayed for modern remote-tracking refs
> gitk: use type "remote" for remote-tracking references
> gitk: introduce a constant otherrefbgcolor
> gitk: add a configuration setting `remoterefbgcolor`
> gitk: change the default colors for remote-tracking references
>
> gitk | 114 ++++++++++++++++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 76 insertions(+), 38 deletions(-)
>
^ permalink raw reply
* Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"
From: Siddharth Kannan @ 2017-02-06 18:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, pranit.bauva, Matthieu.Moy, peff, pclouds, sandals
In-Reply-To: <xmqqtw882n08.fsf@gitster.mtv.corp.google.com>
Hey Junio, I did some more digging into the codepath:
On Sun, Feb 05, 2017 at 04:15:03PM -0800, Junio C Hamano wrote:
>
> A correct solution needs to know if the argument is at the position
> where a revision (or revision range) is expected and then give the
> tip of the previous branch when it sees "-" (and other combinations
> this patch tries to cover). In other words, the parser always knows
> what it is parsing, and if and only if it is parsing a rev, react to
> "-" and think "ah, the user wants me to use the tip of the previous
> branch".
>
> But the code that knows that it expects to see a revision already
> exists, and it is the real parser. In the above snippet,
> setup_revisions() is the one that does the real parsing of argv[].
> The code there knows when it wants to see a rev, and takes argv[i]
> and turns into an object to call add_pending_object(). That codepath
> may not yet know that "-" means the tip of the previous branch, and
> that is where the change needs to go.
Inside setup_revisions, it tries to parse arguments and options. In
there, is this line of code:
if (*arg == '-') {
Once control enters this branch, it will either parse the argument as
an option / pseudo-option, or simply leave this argument as is in the
argv[] array and move forward with the other arguments.
So, first I need to teach setup_revisions that something starting with
a "-" might be a revision or a revision range.
After this, going further down the codepath, in
revision.c:handle_revision_arg:
The argument is parsed to find out if it is of the form
rev1...rev2 or rev1..rev2 or just rev1, etc.
(a) -> If `..` or `...` was found, then two pointers "this" and "next"
now hold the from and to revisions, and the function
get_sha1_committish is called on them. In case both were found to be
committish, then the char pointers now hold the sha1 in them, they are
parsed into objects.
(b) -> Else look for "r1^@", "r1^!" (This could be "-^@", "-^!") To
get r1, again the function get_sha1_committish is called with only r1
as the parameter.
(c) -> Else look for "r1^-"
(d) -> Else look for the argument using the same get_sha1_committish
function (It any "^" was present in it, it has already been noted and
removed)
Cases (a), (b) and (d) can be handled by putting this inside
get_sha1_committish. (Further discussion about that below)
Case (c) is a bit confusing. This could be something like "-^-", and
something like "^-" could mean "Not commits on previous branch" or it
could mean "All commits on this branch except for the parent of HEAD"
Please tell me if this is confusing or undesired altogether.
Personally, I feel that people who have been using "^-" would be
very confused if it's behaviour changed.
So, all the code till now points at adding the patch for "-" = "@{-1}"
inside get_sha1_committish or downstream from there.
get_sha1_committish
-> get_sha1_with_context
-> get_sha1_with_context_1
-> get_sha1_1
-> peel_onion -> calling get_sha1_basic again with the ref
only (after peeling)
-> get_sha1_basic -> includes parsing of "@{-N}" type revs. So,
this indicates that if we can convert the "-" appropriately
before this point, then it would be good.
-> get_short_sha1
So, this patch reduces to the following 2 tasks:
1. Teach setup_revisions that something starting with "-" can be an
argument as well
2. Teach get_sha1_basic that "-" means the tip of the previous branch
perhaps by replacing it with "@{-1}" just before the reflog parsing is
done
> A correct solution will be a lot more involved, of course, and I
> think it will be larger than a reasonable microproject for people
> new to the codebase.
So true :) I had spent a fair bit of time already on my previous patch,
and I thought I might as well complete my research into this, and send
this write-up to the mailing list, so that I could write a patch some
time later. In case you would prefer for me to not work on this
anymore because I am new to the codebase, I will leave it at this.
- Siddharth Kannan
^ permalink raw reply
* Re: [PATCH] difftool: fix bug when printing usage
From: Junio C Hamano @ 2017-02-06 18:13 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: David Aguilar, Git ML, Denton Liu
In-Reply-To: <alpine.DEB.2.20.1702061716120.3496@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> +test_expect_success 'basic usage requires no repo' '
>> + lines=$(git difftool -h | grep ^usage: | wc -l) &&
>> + test "$lines" -eq 1 &&
>
> It may be easier to debug future breakages if you wrote
>
> git difftool -h | grep ^usage: >output &&
> test_line_count = 1 output &&
> or even better (changing the semantics now):
>
> test_expect_code 129 git difftool -h >output &&
> grep ^usage: output &&
True.
>> + # create a ceiling directory to prevent Git from finding a repo
>> + mkdir -p not/repo &&
>> + ceiling="$PWD/not" &&
>> + lines=$(cd not/repo &&
>> + GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
>> + grep ^usage: | wc -l) &&
>> + test "$lines" -eq 1 &&
>
> Likewise, this would become
>
> GIT_CEILING_DIRECTORIES="$PWD/not" \
> test_expect_code 129 git -C not/repo difftool -h >output &&
> grep ^usage: output
I agree with the intent, but the execution here is "Not quite".
test_expect_code being a shell function, it does not take the
"one-shot environment assignment for this single invocation," like
external commands do.
> More importantly: When I read $PWD all kinds of alarm bells go off in my
> head, as I immediately think of all the issues we have on Windows due to
> Git's regression test using POSIX paths all over the place.
And we appreciate that somebody who is more familiar with the issue
is watching ;-).
> Insofar as I am the author of the builtin difftool:
>
> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
OK, thanks.
^ permalink raw reply
* Re: [PATCH v3] parse-remote: remove reference to unused op_prep
From: Junio C Hamano @ 2017-02-06 18:17 UTC (permalink / raw)
To: Siddharth Kannan; +Cc: Pranit Bauva, Git List
In-Reply-To: <20170206022804.GB3323@ubuntu-512mb-blr1-01.localdomain>
Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> Hey Pranit,
> On Sun, Feb 05, 2017 at 02:45:46AM +0530, Pranit Bauva wrote:
>> Hey Siddharth,
>>
>> On Sat, Feb 4, 2017 at 8:01 PM, Siddharth Kannan
>> <kannan.siddharth12@gmail.com> wrote:
>> > The error_on_missing_default_upstream helper function learned to
>> > take op_prep argument with 15a147e618 ("rebase: use @{upstream}
>> > if no upstream specified", 2011-02-09), but as of 045fac5845
>> > ("i18n: git-parse-remote.sh: mark strings for translation",
>> > 2016-04-19), the argument is no longer used. Remove it.
>> >
>> > Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
>>
>> This looks good to me! Thanks :)
>>
>> Regards,
>> Pranit Bauva
>
> Should I send this patch with "To:" set to Junio and "Cc:" set to the
> mailing list, as mentioend in the SubmittingPatches document?
Nah, I was watching the discussion from the sideline. I'll pick it
up after doing one final read on the patch myself.
Thanks, both.
^ 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