From: Stefan Beller <hidden> Date: 2017-10-28 00:44:26
Occasionally a user is given an object hash from a blob as an error message
or other output (e.g. [1]).
It would be useful to get a further description of such a blob, such as
the (commit, path) tuple where this blob was introduced.
This implements the answer in builtin/describe, but I am not sure if that
is the right place. (One office mate argued it could be a "reverse-blame"
that tells you all the trees/commits where the blob is referenced from).
This is RFC for other reasons as well: tests, docs missing.
Any feedback welcome,
thanks,
Stefan
[1] https://stackoverflow.com/questions/10622179/how-to-find-identify-large-files-commits-in-git-history
Stefan Beller (3):
list-objects.c: factor out traverse_trees_and_blobs
revision.h: introduce blob/tree walking in order of the commits
builtin/describe: describe blobs
builtin/describe.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++----
list-objects.c | 48 +++++++++++++++++++++++++++-----------------
revision.h | 3 ++-
3 files changed, 86 insertions(+), 23 deletions(-)
--
2.15.0.rc2.443.gfcc3b81c0a
From: Stefan Beller <hidden> Date: 2017-10-28 00:45:14
With traverse_trees_and_blobs factored out of the main traverse function,
the next patch can introduce an in-order revision walking with ease.
Signed-off-by: Stefan Beller <redacted>
---
list-objects.c | 46 ++++++++++++++++++++++++++++------------------
1 file changed, 28 insertions(+), 18 deletions(-)
From: Johannes Schindelin <hidden> Date: 2017-10-28 17:20:53
Hi Stefan,
On Fri, 27 Oct 2017, Stefan Beller wrote:
This will be useful shortly.
Something tells me that I will hate you in the future when I read this
commit message and lack the context (e.g. when blaming, where I cannot see
the child commits let alone the comment in the Merge commit).
How about:
The functionality to list tree objects in the order they were seen
while traversing the commits will be used in the next commit,
where we teach `git describe` to describe not only commits, but
trees and blobs, too.
The diff itself is amazingly easy to review, and obviously correct.
Ciao,
Dscho
From: Stefan Beller <hidden> Date: 2017-10-29 03:22:45
On Sat, Oct 28, 2017 at 10:20 AM, Johannes Schindelin
[off-list ref] wrote:
Hi Stefan,
On Fri, 27 Oct 2017, Stefan Beller wrote:
quoted
This will be useful shortly.
Something tells me that I will hate you in the future when I read this
commit message and lack the context (e.g. when blaming, where I cannot see
the child commits let alone the comment in the Merge commit).
How about:
The functionality to list tree objects in the order they were seen
while traversing the commits will be used in the next commit,
where we teach `git describe` to describe not only commits, but
trees and blobs, too.
Sounds good. If only we had git-when-merged[1] as some easy upstream
function, then the original commit message would provoke less hate as
future us would have an easier time finding the next commit.
[1] https://github.com/mhagger/git-when-merged
When writing the patch I wondered if this could be useful for other
use cases as well, i.e. do we want to have a command line argument
to trigger this behavior?
Thanks,
Stefan
From: Stefan Beller <hidden> Date: 2017-10-28 00:45:20
Sometimes users are given a hash of an object and they want to
identify it further (ex.: Use verify-pack to find the largest blobs,
but what are these? or [1])
The best identification of a blob hash is done via a its path at a
given commit, which this implements.
[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
Signed-off-by: Stefan Beller <redacted>
---
builtin/describe.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 54 insertions(+), 4 deletions(-)
@@ -282,6 +283,50 @@ static void show_suffix(int depth, const struct object_id *oid)printf("-%d-g%s",depth,find_unique_abbrev(oid->hash,abbrev));}+structblob_descriptor{+structobject_idcurrent_commit;+structobject_idlooking_for;+};++staticvoidprocess_commit(structcommit*commit,void*data)+{+structblob_descriptor*bd=data;++bd->current_commit=commit->object.oid;+}++staticvoidprocess_object(structobject*obj,constchar*name,void*data)+{+structblob_descriptor*bd=data;++if(!oidcmp(&bd->looking_for,&obj->oid))+printf(_("blob %s present at path %s in commit %s\n"),+oid_to_hex(&bd->looking_for),name,+oid_to_hex(&bd->current_commit));+}++staticvoiddescribe_blob(structobject_idoid)+{+structrev_inforevs;+structargv_arrayargs=ARGV_ARRAY_INIT;+structblob_descriptorbd={null_oid,oid};++argv_array_pushl(&args,"internal: The first arg is not parsed",+"--all","--single-worktree","--objects",NULL);++revs.tree_blobs_in_commit_order=1;++init_revisions(&revs,NULL);++if(setup_revisions(args.argc,args.argv,&revs,NULL)>1)+BUG("setup_revisions could not handle all args?");++if(prepare_revision_walk(&revs))+die("revision walk setup failed");++traverse_commit_list(&revs,process_commit,process_object,&bd);+}+staticvoiddescribe(constchar*arg,intlast_one){structobject_idoid;
@@ -295,9 +340,14 @@ static void describe(const char *arg, int last_one)if(get_oid(arg,&oid))die(_("Not a valid object name %s"),arg);-cmit=lookup_commit_reference(&oid);-if(!cmit)-die(_("%s is not a valid '%s' object"),arg,commit_type);+cmit=lookup_commit_reference_gently(&oid,1);+if(!cmit){+if(lookup_blob(&oid))+describe_blob(oid);+else+die(_("%s is not a commit nor blob"),arg);+return;+}n=find_commit_name(&cmit->object.oid);if(n&&(tags||all||n->prio==2)){
From: Johannes Schindelin <hidden> Date: 2017-10-28 17:32:15
Hi Stefan,
On Fri, 27 Oct 2017, Stefan Beller wrote:
Sometimes users are given a hash of an object and they want to identify
it further (ex.: Use verify-pack to find the largest blobs, but what are
these? or [1])
The best identification of a blob hash is done via a its path at a
given commit, which this implements.
[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
@@ -295,9 +340,14 @@ static void describe(const char *arg, int last_one) if (get_oid(arg, &oid)) die(_("Not a valid object name %s"), arg);- cmit = lookup_commit_reference(&oid);- if (!cmit)- die(_("%s is not a valid '%s' object"), arg, commit_type);+ cmit = lookup_commit_reference_gently(&oid, 1);+ if (!cmit) {+ if (lookup_blob(&oid))+ describe_blob(oid);+ else+ die(_("%s is not a commit nor blob"), arg);
s/not/neither/
Nicely done, sir!
I wonder whether it would make sense to extend this to tree objects while
we are at it, but maybe that's an easy up-for-grabs.
Thank you very much!
Dscho
From: Jacob Keller <hidden> Date: 2017-10-28 22:47:35
On Sat, Oct 28, 2017 at 10:32 AM, Johannes Schindelin
[off-list ref] wrote:
Hi Stefan,
Nicely done, sir!
I wonder whether it would make sense to extend this to tree objects while
we are at it, but maybe that's an easy up-for-grabs.
Thank you very much!
Dscho
I'd very much like the same ability for trees and gitlinks as well!
But it does seem it might be fairly easy.
Thanks,
Jake
From: Stefan Beller <hidden> Date: 2017-10-29 03:29:01
On Sat, Oct 28, 2017 at 10:32 AM, Johannes Schindelin
[off-list ref] wrote:
Hi Stefan,
On Fri, 27 Oct 2017, Stefan Beller wrote:
quoted
Sometimes users are given a hash of an object and they want to identify
it further (ex.: Use verify-pack to find the largest blobs, but what are
these? or [1])
The best identification of a blob hash is done via a its path at a
given commit, which this implements.
[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
That was the task I was given. Discussion here turned out that users
mostly only ever care about commit object names, trees and blobs are
only useful when specifically given.
@@ -295,9 +340,14 @@ static void describe(const char *arg, int last_one) if (get_oid(arg, &oid)) die(_("Not a valid object name %s"), arg);- cmit = lookup_commit_reference(&oid);- if (!cmit)- die(_("%s is not a valid '%s' object"), arg, commit_type);+ cmit = lookup_commit_reference_gently(&oid, 1);+ if (!cmit) {+ if (lookup_blob(&oid))+ describe_blob(oid);+ else+ die(_("%s is not a commit nor blob"), arg);
s/not/neither/
Nicely done, sir!
I wonder whether it would make sense to extend this to tree objects while
we are at it, but maybe that's an easy up-for-grabs.
I can look into incorporating that, too. What is the use case though?
(Is there any error message, common enough that users want to
identify trees?)
Thanks for the review,
Stefan
From: Kevin Daudt <hidden> Date: 2017-10-29 12:02:34
On Sat, Oct 28, 2017 at 08:28:54PM -0700, Stefan Beller wrote:
On Sat, Oct 28, 2017 at 10:32 AM, Johannes Schindelin
[off-list ref] wrote:
quoted
[..]
I wonder whether it would make sense to extend this to tree objects while
we are at it, but maybe that's an easy up-for-grabs.
I can look into incorporating that, too. What is the use case though?
(Is there any error message, common enough that users want to
identify trees?)
Thanks for the review,
Stefan
Not sure if it's really helpfulp, but sometimes with corrup repos, git
would complain about a certain object missing or corrupt, where it might
be usefull to find out how it's referenced. Not sure though if this
would work in that case, because the repo is already corrupt.
Kevin
From: Johannes Schindelin <hidden> Date: 2017-10-29 12:07:38
Hi Stefan,
On Sat, 28 Oct 2017, Stefan Beller wrote:
On Sat, Oct 28, 2017 at 10:32 AM, Johannes Schindelin
[off-list ref] wrote:
quoted
I wonder whether it would make sense to extend this to tree objects
while we are at it, but maybe that's an easy up-for-grabs.
I can look into incorporating that, too. What is the use case though?
(Is there any error message, common enough that users want to identify
trees?)
I remember that I wanted to know in the past where that tree object came
from that is now missing (back in the days when worktrees gc'ed away
detached worktree HEADs and their reflogs), but on second thought, I guess
`git describe` would go belly up in that case (die()ing due to the very
same missing tree object)...
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2017-10-28 17:15:47
Hi Stefan,
[I was intrigued enough by your work to postpone to later this coming week
reading the What's cooking email in favor of reviewing your patch series.]
On Fri, 27 Oct 2017, Stefan Beller wrote:
With traverse_trees_and_blobs factored out of the main traverse function,
the next patch can introduce an in-order revision walking with ease.
From: Stefan Beller <hidden> Date: 2017-10-29 03:13:20
On Sat, Oct 28, 2017 at 10:15 AM, Johannes Schindelin
[off-list ref] wrote:
Hi Stefan,
[I was intrigued enough by your work to postpone to later this coming week
reading the What's cooking email in favor of reviewing your patch series.]
On Fri, 27 Oct 2017, Stefan Beller wrote:
quoted
With traverse_trees_and_blobs factored out of the main traverse function,
the next patch can introduce an in-order revision walking with ease.
In the context of one function, it was obvious what `base` meant. Maybe we
can call it `base_path` now?
I was intrigued to keep the base local to the factored out function, but that
would mean, we'd have to allocate memory for it in every call. That I wanted
to avoid, so the only reason to pass it in, is memory management.
base_path sounds good, will rename.
Thanks for the review!
Stefan
From: Johannes Schindelin <hidden> Date: 2017-10-28 16:04:28
Hi Stefan,
On Fri, 27 Oct 2017, Stefan Beller wrote:
Occasionally a user is given an object hash from a blob as an error message
or other output (e.g. [1]).
It would be useful to get a further description of such a blob, such as
the (commit, path) tuple where this blob was introduced.
This implements the answer in builtin/describe, but I am not sure if that
is the right place. (One office mate argued it could be a "reverse-blame"
that tells you all the trees/commits where the blob is referenced from).
This is RFC for other reasons as well: tests, docs missing.
Any feedback welcome,
As you ask so nicely, I'll just cheer (I do not have time to review it
right now, but I really, really wanted this, I even started working on the
name-rev side of things some time ago but eventually had to let things
slide).
Ciao,
Dscho
From: Stefan Beller <hidden> Date: 2017-10-31 00:34:06
This is not an RFC any more, but a serious series.
Occasionally a user is given an object hash from a blob as an error message
or other output (e.g. [1]).
It would be useful to get a further description of such a blob, such as
the (commit, path) tuple where this blob was introduced.
This implements the answer in builtin/describe,
however the heuristics are weak. See patch 6 for details.
Any feedback welcome,
Thanks,
Stefan
[1] https://stackoverflow.com/questions/10622179/how-to-find-identify-large-files-commits-in-git-history
Stefan Beller (7):
list-objects.c: factor out traverse_trees_and_blobs
revision.h: introduce blob/tree walking in order of the commits
builtin/describe.c: rename `oid` to avoid variable shadowing
builtin/describe.c: print debug statements earlier
builtin/describe.c: factor out describe_commit
builtin/describe.c: describe a blob
t6120: fix typo in test name
Documentation/git-describe.txt | 12 +++-
builtin/describe.c | 125 ++++++++++++++++++++++++++++++++---------
list-objects.c | 50 ++++++++++-------
revision.c | 2 +
revision.h | 3 +-
t/t6100-rev-list-in-order.sh | 44 +++++++++++++++
t/t6120-describe.sh | 17 +++++-
7 files changed, 203 insertions(+), 50 deletions(-)
create mode 100755 t/t6100-rev-list-in-order.sh
--
2.15.0.rc2.443.gfcc3b81c0a
From: Stefan Beller <hidden> Date: 2017-10-31 00:34:08
With traverse_trees_and_blobs factored out of the main traverse function,
the next patch can introduce an in-order revision walking with ease.
The variable holding the base path is only used in the newly factored out
function `traverse_trees_and_blobs`, however we keep its scope to
`traverse_commit_list` to keep the number of invocations for memory
allocations and release to one per commit traversal. Rename the variable
to `base_path` for clarity.
Signed-off-by: Stefan Beller <redacted>
---
list-objects.c | 48 +++++++++++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 19 deletions(-)
From: Stefan Beller <hidden> Date: 2017-10-31 00:34:16
In the next patch we'll learn how to describe more than just commits,
so factor out describing commits into its own function. That will make
the next patches easy as we still need to describe a commit as part of
describing blobs.
While factoring out the functionality to describe_commit, make sure
that any output to stdout is put into a strbuf, which we can print
afterwards, using puts which also adds the line ending.
Signed-off-by: Stefan Beller <redacted>
---
builtin/describe.c | 63 ++++++++++++++++++++++++++++++++----------------------
1 file changed, 37 insertions(+), 26 deletions(-)
@@ -293,26 +292,18 @@ static void describe(const char *arg, int last_one)unsignedlongseen_commits=0;unsignedintunannotated_cnt=0;-if(debug)-fprintf(stderr,_("describe %s\n"),arg);--if(get_oid(arg,&oid))-die(_("Not a valid object name %s"),arg);-cmit=lookup_commit_reference(&oid);-if(!cmit)-die(_("%s is not a valid '%s' object"),arg,commit_type);+cmit=lookup_commit_reference(oid);n=find_commit_name(&cmit->object.oid);if(n&&(tags||all||n->prio==2)){/**Exactmatchtoanexistingref.*/-display_name(n);+append_name(n,dst);if(longformat)-show_suffix(0,n->tag?&n->tag->tagged->oid:&oid);+append_suffix(0,n->tag?&n->tag->tagged->oid:oid,dst);if(suffix)-printf("%s",suffix);-printf("\n");+strbuf_addstr(dst,suffix);return;}
@@ -437,15 +427,36 @@ static void describe(const char *arg, int last_one)}}-display_name(all_matches[0].name);+append_name(all_matches[0].name,dst);if(abbrev)-show_suffix(all_matches[0].depth,&cmit->object.oid);+append_suffix(all_matches[0].depth,&cmit->object.oid,dst);if(suffix)-printf("%s",suffix);-printf("\n");+strbuf_addstr(dst,suffix);+}++staticvoiddescribe(constchar*arg,intlast_one)+{+structobject_idoid;+structcommit*cmit;+structstrbufsb=STRBUF_INIT;++if(debug)+fprintf(stderr,_("describe %s\n"),arg);++if(get_oid(arg,&oid))+die(_("Not a valid object name %s"),arg);+cmit=lookup_commit_reference(&oid);+if(!cmit)+die(_("%s is not a valid '%s' object"),arg,commit_type);++describe_commit(&oid,&sb);++puts(sb.buf);if(!last_one)clear_commit_marks(cmit,-1);++strbuf_release(&sb);}intcmd_describe(intargc,constchar**argv,constchar*prefix)
From: Stefan Beller <hidden> Date: 2017-10-31 00:34:18
For debuggers aid we'd want to print debug statements early, so
introduce a new line in the debug output that describes the whole
function, and the change the next debug output to describe why we need
to search. Conveniently drop the arg from the second line; which will
be useful in a follow up commit, that refactors the describe function.
Signed-off-by: Stefan Beller <redacted>
---
builtin/describe.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
@@ -293,6 +293,9 @@ static void describe(const char *arg, int last_one)unsignedlongseen_commits=0;unsignedintunannotated_cnt=0;+if(debug)+fprintf(stderr,_("describe %s\n"),arg);+if(get_oid(arg,&oid))die(_("Not a valid object name %s"),arg);cmit=lookup_commit_reference(&oid);
@@ -316,7 +319,7 @@ static void describe(const char *arg, int last_one)if(!max_candidates)die(_("no tag exactly matches '%s'"),oid_to_hex(&cmit->object.oid));if(debug)-fprintf(stderr,_("searching to describe %s\n"),arg);+fprintf(stderr,_("No exact match on refs or tags, searching to describe\n"));if(!have_util){structhashmap_iteriter;
From: Stefan Beller <hidden> Date: 2017-10-31 00:34:23
Sometimes users are given a hash of an object and they want to
identify it further (ex.: Use verify-pack to find the largest blobs,
but what are these? or [1])
"This is an interesting endeavor, because describing things is hard."
-- me, upon writing this patch.
When describing commits, we try to anchor them to tags or refs, as these
are conceptually on a higher level than the commit. And if there is no ref
or tag that matches exactly, we're out of luck. So we employ a heuristic
to make up a name for the commit. These names are ambivalent, there might
be different tags or refs to anchor to, and there might be different
path in the DAG to travel to arrive at the commit precisely.
When describing a blob, we want to describe the blob from a higher layer
as well, which is a tuple of (commit, deep/path) as the tree objects
involved are rather uninteresting. The same blob can be referenced by
multiple commits, so how we decide which commit to use? This patch
implements a rather naive approach on this: As there are no back pointers
from blobs to commits in which the blob occurs, we'll start walking from
any tips available, listing the blobs in-order of the commit and once we
found the blob, we'll take the first commit that listed the blob. For
source code this is likely not the first commit that introduced the blob,
but rather the latest commit that contained the blob. For example:
git describe v0.99:Makefile
v0.99-5-gab6625e06a:Makefile
tells us the latest commit that contained the Makefile as it was in tag
v0.99 is commit v0.99-5-gab6625e06a (and at the same path), as the next
commit on top v0.99-6-gb1de9de2b9 ([PATCH] Bootstrap "make dist",
2005-07-11) touches the Makefile.
Let's see how this description turns out, if it is useful in day-to-day
use as I have the intuition that we'd rather want to see the *first*
commit that this blob was introduced to the repository (which can be
achieved easily by giving the `--reverse` flag in the describe_blob rev
walk).
[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
Signed-off-by: Stefan Beller <redacted>
---
Documentation/git-describe.txt | 12 +++++++-
builtin/describe.c | 65 ++++++++++++++++++++++++++++++++++++++----
t/t6120-describe.sh | 15 ++++++++++
3 files changed, 86 insertions(+), 6 deletions(-)
@@ -3,7 +3,7 @@ git-describe(1) NAME -----git-describe - Describe a commit using the most recent tag reachable from it+git-describe - Describe a commit or blob using the most recent tag reachable from it SYNOPSIS
@@ -24,6 +24,16 @@ By default (without --all or --tags) `git describe` only shows annotated tags. For more information about creating annotated tags see the -a and -s options to linkgit:git-tag[1].+If the given `<commit-ish>` refers to a blob, it will be described+as `<commit-description>:<path>`, such that the blob can be found+at `<path>` in the `<commit-ish`. Note, that the commit is likely+not the commit that introduced the blob, but the one that was found+first; to find the commit that introduced the blob, you need to find+the commit that last touched the path, e.g.+`git log <commit-description> -- <path>`.+As blobs do not point at the commits they are contained in,+describing blobs is slow as we have to walk the whole graph.+ OPTIONS ------- <commit-ish>...::
@@ -434,6 +436,54 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)strbuf_addstr(dst,suffix);}+structprocess_commit_data{+structobject_idcurrent_commit;+structobject_idlooking_for;+structstrbuf*dst;+};++staticvoidprocess_commit(structcommit*commit,void*data)+{+structprocess_commit_data*pcd=data;+pcd->current_commit=commit->object.oid;+}++staticvoidprocess_object(structobject*obj,constchar*path,void*data)+{+structprocess_commit_data*pcd=data;++if(!oidcmp(&pcd->looking_for,&obj->oid)&&!pcd->dst->len){+reset_revision_walk();+describe_commit(&pcd->current_commit,pcd->dst);+strbuf_addf(pcd->dst,":%s",path);+}+}++staticvoiddescribe_blob(structobject_idoid,structstrbuf*dst)+{+structrev_inforevs;+structargv_arrayargs=ARGV_ARRAY_INIT;+structprocess_commit_datapcd={null_oid,oid,dst};++argv_array_pushl(&args,"internal: The first arg is not parsed",+"--all","--reflog",/* as many starting points as possible */+/* NEEDSWORK: --all is incompatible with worktrees for now: */+"--single-worktree",+"--objects",+"--in-commit-order",+NULL);++init_revisions(&revs,NULL);+if(setup_revisions(args.argc,args.argv,&revs,NULL)>1)+BUG("setup_revisions could not handle all args?");++if(prepare_revision_walk(&revs))+die("revision walk setup failed");++traverse_commit_list(&revs,process_commit,process_object,&pcd);+reset_revision_walk();+}+staticvoiddescribe(constchar*arg,intlast_one){structobject_idoid;
@@ -445,11 +495,16 @@ static void describe(const char *arg, int last_one)if(get_oid(arg,&oid))die(_("Not a valid object name %s"),arg);-cmit=lookup_commit_reference(&oid);-if(!cmit)-die(_("%s is not a valid '%s' object"),arg,commit_type);+cmit=lookup_commit_reference_gently(&oid,1);-describe_commit(&oid,&sb);+if(cmit){+describe_commit(&oid,&sb);+}else{+if(lookup_blob(&oid))+describe_blob(oid,&sb);+else+die(_("%s is neither a commit nor blob"),arg);+}puts(sb.buf);
@@ -310,6 +310,21 @@ test_expect_success 'describe ignoring a borken submodule' 'grepbrokenout'+test_expect_success'describe a blob at a tag''+echo"make it a unique blob">file&&+gitaddfile&&gitcommit-m"content in file"&&+gittag-a-m"latest annotated tag"unique-file&&+gitdescribeHEAD:file>actual&&+echo"unique-file:file">expect&&+test_cmpexpectactual+'++test_expect_success'describe a surviving blob''+gitcommit--allow-empty-m"empty commit"&&+gitdescribeHEAD:file>actual&&+grepunique-file-1-gactual+'+ test_expect_failureULIMIT_STACK_SIZE'name-rev works in a deep repo''i=1&&whiletest$i-lt8000
From: Stefan Beller <hidden> Date: 2017-10-31 00:34:32
The function `describe` has already a variable named `oid` declared at
the beginning of the function for an object id. Do now shadow that
variable with a pointer to an object id.
Signed-off-by: Stefan Beller <redacted>
---
builtin/describe.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -392,11 +392,11 @@ static void describe(const char *arg, int last_one)if(unannotated_cnt)die(_("No annotated tags can describe '%s'.\n""However, there were unannotated tags: try --tags."),-oid_to_hex(oid));+oid_to_hex(cmit_oid));elsedie(_("No tags can describe '%s'.\n""Try --always, or create some tags."),-oid_to_hex(oid));+oid_to_hex(cmit_oid));}QSORT(all_matches,match_cnt,compare_pt);
From: Jacob Keller <hidden> Date: 2017-10-31 08:15:35
On October 30, 2017 5:33:47 PM PDT, Stefan Beller [off-list ref] wrote:
The function `describe` has already a variable named `oid` declared at
the beginning of the function for an object id. Do now shadow that
Nit, s/now/not/
quoted hunk
variable with a pointer to an object id.
Signed-off-by: Stefan Beller <redacted>
---
builtin/describe.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Stefan Beller <hidden> Date: 2017-10-31 00:34:39
The functionality to list tree objects in the order they were seen
while traversing the commits will be used in the next commit,
where we teach `git describe` to describe not only commits, but
trees and blobs, too.
Helped-by: Johannes Schindelin [off-list ref]
Signed-off-by: Stefan Beller <redacted>
---
list-objects.c | 2 ++
revision.c | 2 ++
revision.h | 3 ++-
t/t6100-rev-list-in-order.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 50 insertions(+), 1 deletion(-)
create mode 100755 t/t6100-rev-list-in-order.sh
From: Stefan Beller <hidden> Date: 2017-10-31 21:19:00
v2:
* other variable names in patch v1, the commit message explains the
unusual strategy for the scratch pad variable, + assert
* less ugly test in p2
* typofix in p3 commit msg
* patch 4 (debug printing) unchanged, awaiting discussion to start/settle.
* rephrased the man page in p6.
Thanks,
Stefan
v1:
This is not an RFC any more, but a serious series.
Occasionally a user is given an object hash from a blob as an error message
or other output (e.g. [1]).
It would be useful to get a further description of such a blob, such as
the (commit, path) tuple where this blob was introduced.
This implements the answer in builtin/describe,
however the heuristics are weak. See patch 6 for details.
Any feedback welcome,
Thanks,
Stefan
Stefan Beller (7):
list-objects.c: factor out traverse_trees_and_blobs
revision.h: introduce blob/tree walking in order of the commits
builtin/describe.c: rename `oid` to avoid variable shadowing
builtin/describe.c: print debug statements earlier
builtin/describe.c: factor out describe_commit
builtin/describe.c: describe a blob
t6120: fix typo in test name
Documentation/git-describe.txt | 13 +++-
Documentation/rev-list-options.txt | 5 ++
builtin/describe.c | 125 ++++++++++++++++++++++++++++---------
list-objects.c | 52 +++++++++------
revision.c | 2 +
revision.h | 3 +-
t/t6100-rev-list-in-order.sh | 46 ++++++++++++++
t/t6120-describe.sh | 17 ++++-
8 files changed, 213 insertions(+), 50 deletions(-)
create mode 100755 t/t6100-rev-list-in-order.sh
--
2.15.0.rc2.443.gfcc3b81c0a
From: Stefan Beller <hidden> Date: 2017-10-31 21:19:02
With traverse_trees_and_blobs factored out of the main traverse function,
the next patch can introduce an in-order revision walking with ease.
In the next patch we'll call `traverse_trees_and_blobs` from within the
loop walking the commits, such that we'll have one invocation of that
function per commit. That is why we do not want to have memory allocations
in that function, such as we'd have if we were to use a strbuf locally.
Pass a strbuf from traverse_commit_list into the blob and tree traversing
function as a scratch pad that only needs to be allocated once.
Signed-off-by: Stefan Beller <redacted>
---
list-objects.c | 50 +++++++++++++++++++++++++++++++-------------------
1 file changed, 31 insertions(+), 19 deletions(-)
From: Stefan Beller <hidden> Date: 2017-10-31 21:19:08
For debuggers aid we'd want to print debug statements early, so
introduce a new line in the debug output that describes the whole
function, and the change the next debug output to describe why we need
to search. Conveniently drop the arg from the second line; which will
be useful in a follow up commit, that refactors the describe function.
Signed-off-by: Stefan Beller <redacted>
---
builtin/describe.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
@@ -293,6 +293,9 @@ static void describe(const char *arg, int last_one)unsignedlongseen_commits=0;unsignedintunannotated_cnt=0;+if(debug)+fprintf(stderr,_("describe %s\n"),arg);+if(get_oid(arg,&oid))die(_("Not a valid object name %s"),arg);cmit=lookup_commit_reference(&oid);
@@ -316,7 +319,7 @@ static void describe(const char *arg, int last_one)if(!max_candidates)die(_("no tag exactly matches '%s'"),oid_to_hex(&cmit->object.oid));if(debug)-fprintf(stderr,_("searching to describe %s\n"),arg);+fprintf(stderr,_("No exact match on refs or tags, searching to describe\n"));if(!have_util){structhashmap_iteriter;
From: Eric Sunshine <hidden> Date: 2017-10-31 21:31:18
On Tue, Oct 31, 2017 at 5:18 PM, Stefan Beller [off-list ref] wrote:
For debuggers aid we'd want to print debug statements early, so
introduce a new line in the debug output that describes the whole
function, and the change the next debug output to describe why we need
s/and the/and/
...or...
s/and the/and then/
to search. Conveniently drop the arg from the second line; which will
be useful in a follow up commit, that refactors the describe function.
Signed-off-by: Stefan Beller <redacted>
From: Stefan Beller <hidden> Date: 2017-10-31 21:19:14
The functionality to list tree objects in the order they were seen
while traversing the commits will be used in the next commit,
where we teach `git describe` to describe not only commits, but
trees and blobs, too.
Signed-off-by: Stefan Beller <redacted>
---
Documentation/rev-list-options.txt | 5 +++++
list-objects.c | 2 ++
revision.c | 2 ++
revision.h | 3 ++-
t/t6100-rev-list-in-order.sh | 46 ++++++++++++++++++++++++++++++++++++++
5 files changed, 57 insertions(+), 1 deletion(-)
create mode 100755 t/t6100-rev-list-in-order.sh
@@ -686,6 +686,11 @@ ifdef::git-rev-list[] all object IDs which I need to download if I have the commit object _bar_ but not _foo_''.+--in-commit-order::+ Print tree and blob ids in order of the commits. The tree+ and blob ids are printed after they are first referenced+ by a commit.+ --objects-edge:: Similar to `--objects`, but also print the IDs of excluded commits prefixed with a ``-'' character. This is used by
From: Stefan Beller <hidden> Date: 2017-10-31 21:19:16
Sometimes users are given a hash of an object and they want to
identify it further (ex.: Use verify-pack to find the largest blobs,
but what are these? or [1])
"This is an interesting endeavor, because describing things is hard."
-- me, upon writing this patch.
When describing commits, we try to anchor them to tags or refs, as these
are conceptually on a higher level than the commit. And if there is no ref
or tag that matches exactly, we're out of luck. So we employ a heuristic
to make up a name for the commit. These names are ambivalent, there might
be different tags or refs to anchor to, and there might be different
path in the DAG to travel to arrive at the commit precisely.
When describing a blob, we want to describe the blob from a higher layer
as well, which is a tuple of (commit, deep/path) as the tree objects
involved are rather uninteresting. The same blob can be referenced by
multiple commits, so how we decide which commit to use? This patch
implements a rather naive approach on this: As there are no back pointers
from blobs to commits in which the blob occurs, we'll start walking from
any tips available, listing the blobs in-order of the commit and once we
found the blob, we'll take the first commit that listed the blob. For
source code this is likely not the first commit that introduced the blob,
but rather the latest commit that contained the blob. For example:
git describe v0.99:Makefile
v0.99-5-gab6625e06a:Makefile
tells us the latest commit that contained the Makefile as it was in tag
v0.99 is commit v0.99-5-gab6625e06a (and at the same path), as the next
commit on top v0.99-6-gb1de9de2b9 ([PATCH] Bootstrap "make dist",
2005-07-11) touches the Makefile.
Let's see how this description turns out, if it is useful in day-to-day
use as I have the intuition that we'd rather want to see the *first*
commit that this blob was introduced to the repository (which can be
achieved easily by giving the `--reverse` flag in the describe_blob rev
walk).
[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
Signed-off-by: Stefan Beller <redacted>
---
Documentation/git-describe.txt | 13 ++++++++-
builtin/describe.c | 65 ++++++++++++++++++++++++++++++++++++++----
t/t6120-describe.sh | 15 ++++++++++
3 files changed, 87 insertions(+), 6 deletions(-)
@@ -3,7 +3,7 @@ git-describe(1) NAME -----git-describe - Describe a commit using the most recent tag reachable from it+git-describe - Describe a commit or blob using the graph relations SYNOPSIS
@@ -24,6 +25,16 @@ By default (without --all or --tags) `git describe` only shows annotated tags. For more information about creating annotated tags see the -a and -s options to linkgit:git-tag[1].+If the given object refers to a blob, it will be described+as `<commit-ish>:<path>`, such that the blob can be found+at `<path>` in the `<commit-ish>`. Note, that the commit is likely+not the commit that introduced the blob, but the one that was found+first; to find the commit that introduced the blob, you need to find+the commit that last touched the path, e.g.+`git log <commit-description> -- <path>`.+As blobs do not point at the commits they are contained in,+describing blobs is slow as we have to walk the whole graph.+ OPTIONS ------- <commit-ish>...::
@@ -434,6 +436,54 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)strbuf_addstr(dst,suffix);}+structprocess_commit_data{+structobject_idcurrent_commit;+structobject_idlooking_for;+structstrbuf*dst;+};++staticvoidprocess_commit(structcommit*commit,void*data)+{+structprocess_commit_data*pcd=data;+pcd->current_commit=commit->object.oid;+}++staticvoidprocess_object(structobject*obj,constchar*path,void*data)+{+structprocess_commit_data*pcd=data;++if(!oidcmp(&pcd->looking_for,&obj->oid)&&!pcd->dst->len){+reset_revision_walk();+describe_commit(&pcd->current_commit,pcd->dst);+strbuf_addf(pcd->dst,":%s",path);+}+}++staticvoiddescribe_blob(structobject_idoid,structstrbuf*dst)+{+structrev_inforevs;+structargv_arrayargs=ARGV_ARRAY_INIT;+structprocess_commit_datapcd={null_oid,oid,dst};++argv_array_pushl(&args,"internal: The first arg is not parsed",+"--all","--reflog",/* as many starting points as possible */+/* NEEDSWORK: --all is incompatible with worktrees for now: */+"--single-worktree",+"--objects",+"--in-commit-order",+NULL);++init_revisions(&revs,NULL);+if(setup_revisions(args.argc,args.argv,&revs,NULL)>1)+BUG("setup_revisions could not handle all args?");++if(prepare_revision_walk(&revs))+die("revision walk setup failed");++traverse_commit_list(&revs,process_commit,process_object,&pcd);+reset_revision_walk();+}+staticvoiddescribe(constchar*arg,intlast_one){structobject_idoid;
@@ -445,11 +495,16 @@ static void describe(const char *arg, int last_one)if(get_oid(arg,&oid))die(_("Not a valid object name %s"),arg);-cmit=lookup_commit_reference(&oid);-if(!cmit)-die(_("%s is not a valid '%s' object"),arg,commit_type);+cmit=lookup_commit_reference_gently(&oid,1);-describe_commit(&oid,&sb);+if(cmit){+describe_commit(&oid,&sb);+}else{+if(lookup_blob(&oid))+describe_blob(oid,&sb);+else+die(_("%s is neither a commit nor blob"),arg);+}puts(sb.buf);
@@ -310,6 +310,21 @@ test_expect_success 'describe ignoring a borken submodule' 'grepbrokenout'+test_expect_success'describe a blob at a tag''+echo"make it a unique blob">file&&+gitaddfile&&gitcommit-m"content in file"&&+gittag-a-m"latest annotated tag"unique-file&&+gitdescribeHEAD:file>actual&&+echo"unique-file:file">expect&&+test_cmpexpectactual+'++test_expect_success'describe a surviving blob''+gitcommit--allow-empty-m"empty commit"&&+gitdescribeHEAD:file>actual&&+grepunique-file-1-gactual+'+ test_expect_failureULIMIT_STACK_SIZE'name-rev works in a deep repo''i=1&&whiletest$i-lt8000
From: Eric Sunshine <hidden> Date: 2017-10-31 21:49:10
On Tue, Oct 31, 2017 at 5:18 PM, Stefan Beller [off-list ref] wrote:
When describing commits, we try to anchor them to tags or refs, as these
are conceptually on a higher level than the commit. And if there is no ref
or tag that matches exactly, we're out of luck. So we employ a heuristic
to make up a name for the commit. These names are ambivalent, there might
@@ -445,11 +495,16 @@ static void describe(const char *arg, int last_one) if (get_oid(arg, &oid)) die(_("Not a valid object name %s"), arg);- cmit = lookup_commit_reference(&oid);- if (!cmit)- die(_("%s is not a valid '%s' object"), arg, commit_type);+ cmit = lookup_commit_reference_gently(&oid, 1);- describe_commit(&oid, &sb);+ if (cmit) {+ describe_commit(&oid, &sb);+ } else {+ if (lookup_blob(&oid))+ describe_blob(oid, &sb);+ else+ die(_("%s is neither a commit nor blob"), arg);+ }
Not at all worth a re-roll, but less nesting and a bit less noisy:
if (cmt)
describe_commit(...);
else if (lookup_blob(...))
describe_blob(...);
else
die(...);
From: Stefan Beller <hidden> Date: 2017-11-01 19:51:41
On Tue, Oct 31, 2017 at 2:49 PM, Eric Sunshine [off-list ref] wrote:
quoted
to make up a name for the commit. These names are ambivalent, there might
I guess you meant s/ambivalent/ambiguous/ ?
Indeed!
ambivalent
early 20th century: from ambivalence (from German Ambivalenz ),
on the pattern of equivalent.
In German ambivalent (~synonym to mehrdeutig, "multi meaning")
actually means ambiguous, one of the false friends!
Thanks for pointing out.
From: Stefan Beller <hidden> Date: 2017-10-31 21:19:19
The function `describe` has already a variable named `oid` declared at
the beginning of the function for an object id. Do not shadow that
variable with a pointer to an object id.
Signed-off-by: Stefan Beller <redacted>
---
builtin/describe.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -392,11 +392,11 @@ static void describe(const char *arg, int last_one)if(unannotated_cnt)die(_("No annotated tags can describe '%s'.\n""However, there were unannotated tags: try --tags."),-oid_to_hex(oid));+oid_to_hex(cmit_oid));elsedie(_("No tags can describe '%s'.\n""Try --always, or create some tags."),-oid_to_hex(oid));+oid_to_hex(cmit_oid));}QSORT(all_matches,match_cnt,compare_pt);
From: Stefan Beller <hidden> Date: 2017-10-31 21:19:21
In the next patch we'll learn how to describe more than just commits,
so factor out describing commits into its own function. That will make
the next patches easy as we still need to describe a commit as part of
describing blobs.
While factoring out the functionality to describe_commit, make sure
that any output to stdout is put into a strbuf, which we can print
afterwards, using puts which also adds the line ending.
Signed-off-by: Stefan Beller <redacted>
---
builtin/describe.c | 63 ++++++++++++++++++++++++++++++++----------------------
1 file changed, 37 insertions(+), 26 deletions(-)
@@ -293,26 +292,18 @@ static void describe(const char *arg, int last_one)unsignedlongseen_commits=0;unsignedintunannotated_cnt=0;-if(debug)-fprintf(stderr,_("describe %s\n"),arg);--if(get_oid(arg,&oid))-die(_("Not a valid object name %s"),arg);-cmit=lookup_commit_reference(&oid);-if(!cmit)-die(_("%s is not a valid '%s' object"),arg,commit_type);+cmit=lookup_commit_reference(oid);n=find_commit_name(&cmit->object.oid);if(n&&(tags||all||n->prio==2)){/**Exactmatchtoanexistingref.*/-display_name(n);+append_name(n,dst);if(longformat)-show_suffix(0,n->tag?&n->tag->tagged->oid:&oid);+append_suffix(0,n->tag?&n->tag->tagged->oid:oid,dst);if(suffix)-printf("%s",suffix);-printf("\n");+strbuf_addstr(dst,suffix);return;}
@@ -437,15 +427,36 @@ static void describe(const char *arg, int last_one)}}-display_name(all_matches[0].name);+append_name(all_matches[0].name,dst);if(abbrev)-show_suffix(all_matches[0].depth,&cmit->object.oid);+append_suffix(all_matches[0].depth,&cmit->object.oid,dst);if(suffix)-printf("%s",suffix);-printf("\n");+strbuf_addstr(dst,suffix);+}++staticvoiddescribe(constchar*arg,intlast_one)+{+structobject_idoid;+structcommit*cmit;+structstrbufsb=STRBUF_INIT;++if(debug)+fprintf(stderr,_("describe %s\n"),arg);++if(get_oid(arg,&oid))+die(_("Not a valid object name %s"),arg);+cmit=lookup_commit_reference(&oid);+if(!cmit)+die(_("%s is not a valid '%s' object"),arg,commit_type);++describe_commit(&oid,&sb);++puts(sb.buf);if(!last_one)clear_commit_marks(cmit,-1);++strbuf_release(&sb);}intcmd_describe(intargc,constchar**argv,constchar*prefix)
From: Stefan Beller <hidden> Date: 2017-11-02 19:42:02
Thanks for the discussion on v2[1].
Interdiff is below, just fixing minor things.
We'll keep the original algorithm for now, deferring an improvement on
the algorithm front towards a future developer.
Thanks,
Stefan
[1] https://public-inbox.org/git/20171031211852.13001-1-sbeller@google.com/
Stefan Beller (7):
t6120: fix typo in test name
list-objects.c: factor out traverse_trees_and_blobs
revision.h: introduce blob/tree walking in order of the commits
builtin/describe.c: rename `oid` to avoid variable shadowing
builtin/describe.c: print debug statements earlier
builtin/describe.c: factor out describe_commit
builtin/describe.c: describe a blob
Documentation/git-describe.txt | 13 +++-
Documentation/rev-list-options.txt | 5 ++
builtin/describe.c | 125 ++++++++++++++++++++++++++++---------
list-objects.c | 52 +++++++++------
revision.c | 2 +
revision.h | 3 +-
t/t6100-rev-list-in-order.sh | 47 ++++++++++++++
t/t6120-describe.sh | 17 ++++-
8 files changed, 214 insertions(+), 50 deletions(-)
create mode 100755 t/t6100-rev-list-in-order.sh
@@ -463,7 +465,7 @@ static void describe_blob(struct object_id oid, struct strbuf *dst){structrev_inforevs;structargv_arrayargs=ARGV_ARRAY_INIT;-structprocess_commit_datapcd={null_oid,oid,dst};+structprocess_commit_datapcd={null_oid,oid,dst,&revs};argv_array_pushl(&args,"internal: The first arg is not parsed","--all","--reflog",/* as many starting points as possible */
@@ -497,14 +499,12 @@ static void describe(const char *arg, int last_one)die(_("Not a valid object name %s"),arg);cmit=lookup_commit_reference_gently(&oid,1);-if(cmit){+if(cmit)describe_commit(&oid,&sb);-}else{-if(lookup_blob(&oid))-describe_blob(oid,&sb);-else-die(_("%s is neither a commit nor blob"),arg);-}+elseif(lookup_blob(&oid))+describe_blob(oid,&sb);+else+die(_("%s is neither a commit nor blob"),arg);puts(sb.buf);
@@ -319,7 +319,7 @@ test_expect_success 'describe a blob at a tag' 'test_cmpexpectactual'-test_expect_success'describe a surviving blob''+test_expect_success'describe a blob with commit-ish''gitcommit--allow-empty-m"empty commit"&&gitdescribeHEAD:file>actual&&grepunique-file-1-gactual
From: Stefan Beller <hidden> Date: 2017-11-02 19:42:07
With traverse_trees_and_blobs factored out of the main traverse function,
the next patch can introduce an in-order revision walking with ease.
In the next patch we'll call `traverse_trees_and_blobs` from within the
loop walking the commits, such that we'll have one invocation of that
function per commit. That is why we do not want to have memory allocations
in that function, such as we'd have if we were to use a strbuf locally.
Pass a strbuf from traverse_commit_list into the blob and tree traversing
function as a scratch pad that only needs to be allocated once.
Signed-off-by: Stefan Beller <redacted>
---
list-objects.c | 50 +++++++++++++++++++++++++++++++-------------------
1 file changed, 31 insertions(+), 19 deletions(-)
From: Stefan Beller <hidden> Date: 2017-11-02 19:42:09
The functionality to list tree objects in the order they were seen
while traversing the commits will be used in the next commit,
where we teach `git describe` to describe not only commits, but
trees and blobs, too.
Signed-off-by: Stefan Beller <redacted>
---
Documentation/rev-list-options.txt | 5 ++++
list-objects.c | 2 ++
revision.c | 2 ++
revision.h | 3 ++-
t/t6100-rev-list-in-order.sh | 47 ++++++++++++++++++++++++++++++++++++++
5 files changed, 58 insertions(+), 1 deletion(-)
create mode 100755 t/t6100-rev-list-in-order.sh
@@ -686,6 +686,11 @@ ifdef::git-rev-list[] all object IDs which I need to download if I have the commit object _bar_ but not _foo_''.+--in-commit-order::+ Print tree and blob ids in order of the commits. The tree+ and blob ids are printed after they are first referenced+ by a commit.+ --objects-edge:: Similar to `--objects`, but also print the IDs of excluded commits prefixed with a ``-'' character. This is used by
@@ -0,0 +1,47 @@+#!/bin/sh++test_description='rev-list testing in-commit-order'++../test-lib.sh++test_expect_success'rev-list --in-commit-order''+forxinonetwothreefour+do+echo$x>$x&&+gitadd$x&&+gitcommit-m"add file $x"||+return1+done&&+forxinfourthree+do+gitrm$x&&+gitcommit-m"remove $x"||+return1+done&&+gitrev-list--in-commit-order--objectsHEAD>actual.raw&&+cut-c1-40>actual<actual.raw&&++gitcat-file--batch-check="%(objectname)">expect.raw<<-\EOF&&+HEAD^{commit}+HEAD^{tree}+HEAD^{tree}:one+HEAD^{tree}:two+HEAD~1^{commit}+HEAD~1^{tree}+HEAD~1^{tree}:three+HEAD~2^{commit}+HEAD~2^{tree}+HEAD~2^{tree}:four+HEAD~3^{commit}+# HEAD~3^{tree} skipped, same as HEAD~1^{tree}+HEAD~4^{commit}+# HEAD~4^{tree} skipped, same as HEAD^{tree}+HEAD~5^{commit}+HEAD~5^{tree}+EOF+grep-v"#">expect<expect.raw&&++test_cmpexpectactual+'++test_done
I would have expected add_pending_tree() above to no longer be invoked.
If it still needs to be invoked, maybe add an explanation in the form of
a comment or commit message.
+test_expect_success 'rev-list --in-commit-order' '
+ for x in one two three four
+ do
+ echo $x >$x &&
+ git add $x &&
+ git commit -m "add file $x" ||
+ return 1
+ done &&
+ for x in four three
+ do
+ git rm $x &&
+ git commit -m "remove $x" ||
+ return 1
+ done &&
+ git rev-list --in-commit-order --objects HEAD >actual.raw &&
+ cut -c 1-40 >actual <actual.raw &&
+
+ git cat-file --batch-check="%(objectname)" >expect.raw <<-\EOF &&
+ HEAD^{commit}
+ HEAD^{tree}
+ HEAD^{tree}:one
+ HEAD^{tree}:two
+ HEAD~1^{commit}
+ HEAD~1^{tree}
+ HEAD~1^{tree}:three
+ HEAD~2^{commit}
+ HEAD~2^{tree}
+ HEAD~2^{tree}:four
+ HEAD~3^{commit}
+ # HEAD~3^{tree} skipped, same as HEAD~1^{tree}
+ HEAD~4^{commit}
+ # HEAD~4^{tree} skipped, same as HEAD^{tree}
+ HEAD~5^{commit}
+ HEAD~5^{tree}
+ EOF
+ grep -v "#" >expect <expect.raw &&
+
+ test_cmp expect actual
+'
Would it be useful to have another test without --in-commit-order, so
that we can see the difference (and ensure that existing behavior is
unchanged)?
From: Stefan Beller <hidden> Date: 2017-11-02 19:42:13
In the next patch we'll learn how to describe more than just commits,
so factor out describing commits into its own function. That will make
the next patches easy as we still need to describe a commit as part of
describing blobs.
While factoring out the functionality to describe_commit, make sure
that any output to stdout is put into a strbuf, which we can print
afterwards, using puts which also adds the line ending.
Signed-off-by: Stefan Beller <redacted>
---
builtin/describe.c | 63 ++++++++++++++++++++++++++++++++----------------------
1 file changed, 37 insertions(+), 26 deletions(-)
@@ -293,26 +292,18 @@ static void describe(const char *arg, int last_one)unsignedlongseen_commits=0;unsignedintunannotated_cnt=0;-if(debug)-fprintf(stderr,_("describe %s\n"),arg);--if(get_oid(arg,&oid))-die(_("Not a valid object name %s"),arg);-cmit=lookup_commit_reference(&oid);-if(!cmit)-die(_("%s is not a valid '%s' object"),arg,commit_type);+cmit=lookup_commit_reference(oid);n=find_commit_name(&cmit->object.oid);if(n&&(tags||all||n->prio==2)){/**Exactmatchtoanexistingref.*/-display_name(n);+append_name(n,dst);if(longformat)-show_suffix(0,n->tag?&n->tag->tagged->oid:&oid);+append_suffix(0,n->tag?&n->tag->tagged->oid:oid,dst);if(suffix)-printf("%s",suffix);-printf("\n");+strbuf_addstr(dst,suffix);return;}
@@ -437,15 +427,36 @@ static void describe(const char *arg, int last_one)}}-display_name(all_matches[0].name);+append_name(all_matches[0].name,dst);if(abbrev)-show_suffix(all_matches[0].depth,&cmit->object.oid);+append_suffix(all_matches[0].depth,&cmit->object.oid,dst);if(suffix)-printf("%s",suffix);-printf("\n");+strbuf_addstr(dst,suffix);+}++staticvoiddescribe(constchar*arg,intlast_one)+{+structobject_idoid;+structcommit*cmit;+structstrbufsb=STRBUF_INIT;++if(debug)+fprintf(stderr,_("describe %s\n"),arg);++if(get_oid(arg,&oid))+die(_("Not a valid object name %s"),arg);+cmit=lookup_commit_reference(&oid);+if(!cmit)+die(_("%s is not a valid '%s' object"),arg,commit_type);++describe_commit(&oid,&sb);++puts(sb.buf);if(!last_one)clear_commit_marks(cmit,-1);++strbuf_release(&sb);}intcmd_describe(intargc,constchar**argv,constchar*prefix)
From: Stefan Beller <hidden> Date: 2017-11-02 19:42:15
For debuggers aid we'd want to print debug statements early, so
introduce a new line in the debug output that describes the whole
function, and then change the next debug output to describe why we
need to search. Conveniently drop the arg from the second line;
which will be useful in a follow up commit, that refactors the\
describe function.
Signed-off-by: Stefan Beller <redacted>
---
builtin/describe.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
@@ -293,6 +293,9 @@ static void describe(const char *arg, int last_one)unsignedlongseen_commits=0;unsignedintunannotated_cnt=0;+if(debug)+fprintf(stderr,_("describe %s\n"),arg);+if(get_oid(arg,&oid))die(_("Not a valid object name %s"),arg);cmit=lookup_commit_reference(&oid);
@@ -316,7 +319,7 @@ static void describe(const char *arg, int last_one)if(!max_candidates)die(_("no tag exactly matches '%s'"),oid_to_hex(&cmit->object.oid));if(debug)-fprintf(stderr,_("searching to describe %s\n"),arg);+fprintf(stderr,_("No exact match on refs or tags, searching to describe\n"));if(!have_util){structhashmap_iteriter;
From: Jonathan Tan <hidden> Date: 2017-11-14 19:55:20
On Thu, 2 Nov 2017 12:41:46 -0700
Stefan Beller [off-list ref] wrote:
quoted hunk
For debuggers aid we'd want to print debug statements early, so
introduce a new line in the debug output that describes the whole
function, and then change the next debug output to describe why we
need to search. Conveniently drop the arg from the second line;
which will be useful in a follow up commit, that refactors the\
describe function.
Signed-off-by: Stefan Beller <redacted>
---
builtin/describe.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Could you explain in the commit message why this wasn't needed before
(if it wasn't needed), and why this is needed now?
quoted hunk
if (get_oid(arg, &oid))
die(_("Not a valid object name %s"), arg);
cmit = lookup_commit_reference(&oid);
@@ -316,7 +319,7 @@ static void describe(const char *arg, int last_one) if (!max_candidates) die(_("no tag exactly matches '%s'"), oid_to_hex(&cmit->object.oid)); if (debug)- fprintf(stderr, _("searching to describe %s\n"), arg);+ fprintf(stderr, _("No exact match on refs or tags, searching to describe\n"));
What is this arg that can be safely dropped?
You mention that it is for convenience (since the describe() function
will be refactored), but could the arg just be passed to the new
function?
From: Stefan Beller <hidden> Date: 2017-11-14 20:00:53
On Tue, Nov 14, 2017 at 11:55 AM, Jonathan Tan [off-list ref] wrote:
On Thu, 2 Nov 2017 12:41:46 -0700
Stefan Beller [off-list ref] wrote:
quoted
For debuggers aid we'd want to print debug statements early, so
introduce a new line in the debug output that describes the whole
function, and then change the next debug output to describe why we
need to search. Conveniently drop the arg from the second line;
which will be useful in a follow up commit, that refactors the\
describe function.
Signed-off-by: Stefan Beller <redacted>
---
builtin/describe.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Could you explain in the commit message why this wasn't needed before
(if it wasn't needed), and why this is needed now?
quoted
if (get_oid(arg, &oid))
die(_("Not a valid object name %s"), arg);
cmit = lookup_commit_reference(&oid);
@@ -316,7 +319,7 @@ static void describe(const char *arg, int last_one) if (!max_candidates) die(_("no tag exactly matches '%s'"), oid_to_hex(&cmit->object.oid)); if (debug)- fprintf(stderr, _("searching to describe %s\n"), arg);+ fprintf(stderr, _("No exact match on refs or tags, searching to describe\n"));
What is this arg that can be safely dropped?
You mention that it is for convenience (since the describe() function
will be refactored), but could the arg just be passed to the new
function?
It could, but I want to avoid that just to print a debugging statement
inside the
function. So I factor the debugging printing out of the function
introduced in the
next patch.
From: Stefan Beller <hidden> Date: 2017-11-02 19:42:17
Sometimes users are given a hash of an object and they want to
identify it further (ex.: Use verify-pack to find the largest blobs,
but what are these? or [1])
"This is an interesting endeavor, because describing things is hard."
-- me, upon writing this patch.
When describing commits, we try to anchor them to tags or refs, as these
are conceptually on a higher level than the commit. And if there is no ref
or tag that matches exactly, we're out of luck. So we employ a heuristic
to make up a name for the commit. These names are ambiguous, there might
be different tags or refs to anchor to, and there might be different
path in the DAG to travel to arrive at the commit precisely.
When describing a blob, we want to describe the blob from a higher layer
as well, which is a tuple of (commit, deep/path) as the tree objects
involved are rather uninteresting. The same blob can be referenced by
multiple commits, so how we decide which commit to use? This patch
implements a rather naive approach on this: As there are no back pointers
from blobs to commits in which the blob occurs, we'll start walking from
any tips available, listing the blobs in-order of the commit and once we
found the blob, we'll take the first commit that listed the blob. For
source code this is likely not the first commit that introduced the blob,
but rather the latest commit that contained the blob. For example:
git describe v0.99:Makefile
v0.99-5-gab6625e06a:Makefile
tells us the latest commit that contained the Makefile as it was in tag
v0.99 is commit v0.99-5-gab6625e06a (and at the same path), as the next
commit on top v0.99-6-gb1de9de2b9 ([PATCH] Bootstrap "make dist",
2005-07-11) touches the Makefile.
Let's see how this description turns out, if it is useful in day-to-day
use as I have the intuition that we'd rather want to see the *first*
commit that this blob was introduced to the repository (which can be
achieved easily by giving the `--reverse` flag in the describe_blob rev
walk).
[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
Signed-off-by: Stefan Beller <redacted>
---
Documentation/git-describe.txt | 13 ++++++++-
builtin/describe.c | 65 ++++++++++++++++++++++++++++++++++++++----
t/t6120-describe.sh | 15 ++++++++++
3 files changed, 87 insertions(+), 6 deletions(-)
@@ -3,7 +3,7 @@ git-describe(1) NAME -----git-describe - Describe a commit using the most recent tag reachable from it+git-describe - Describe a commit or blob using the graph relations SYNOPSIS
@@ -24,6 +25,16 @@ By default (without --all or --tags) `git describe` only shows annotated tags. For more information about creating annotated tags see the -a and -s options to linkgit:git-tag[1].+If the given object refers to a blob, it will be described+as `<commit-ish>:<path>`, such that the blob can be found+at `<path>` in the `<commit-ish>`. Note, that the commit is likely+not the commit that introduced the blob, but the one that was found+first; to find the commit that introduced the blob, you need to find+the commit that last touched the path, e.g.+`git log <commit-description> -- <path>`.+As blobs do not point at the commits they are contained in,+describing blobs is slow as we have to walk the whole graph.+ OPTIONS ------- <commit-ish>...::
@@ -434,6 +436,56 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)strbuf_addstr(dst,suffix);}+structprocess_commit_data{+structobject_idcurrent_commit;+structobject_idlooking_for;+structstrbuf*dst;+structrev_info*revs;+};++staticvoidprocess_commit(structcommit*commit,void*data)+{+structprocess_commit_data*pcd=data;+pcd->current_commit=commit->object.oid;+}++staticvoidprocess_object(structobject*obj,constchar*path,void*data)+{+structprocess_commit_data*pcd=data;++if(!oidcmp(&pcd->looking_for,&obj->oid)&&!pcd->dst->len){+reset_revision_walk();+describe_commit(&pcd->current_commit,pcd->dst);+strbuf_addf(pcd->dst,":%s",path);+pcd->revs->max_count=0;+}+}++staticvoiddescribe_blob(structobject_idoid,structstrbuf*dst)+{+structrev_inforevs;+structargv_arrayargs=ARGV_ARRAY_INIT;+structprocess_commit_datapcd={null_oid,oid,dst,&revs};++argv_array_pushl(&args,"internal: The first arg is not parsed",+"--all","--reflog",/* as many starting points as possible */+/* NEEDSWORK: --all is incompatible with worktrees for now: */+"--single-worktree",+"--objects",+"--in-commit-order",+NULL);++init_revisions(&revs,NULL);+if(setup_revisions(args.argc,args.argv,&revs,NULL)>1)+BUG("setup_revisions could not handle all args?");++if(prepare_revision_walk(&revs))+die("revision walk setup failed");++traverse_commit_list(&revs,process_commit,process_object,&pcd);+reset_revision_walk();+}+staticvoiddescribe(constchar*arg,intlast_one){structobject_idoid;
@@ -445,11 +497,14 @@ static void describe(const char *arg, int last_one)if(get_oid(arg,&oid))die(_("Not a valid object name %s"),arg);-cmit=lookup_commit_reference(&oid);-if(!cmit)-die(_("%s is not a valid '%s' object"),arg,commit_type);+cmit=lookup_commit_reference_gently(&oid,1);-describe_commit(&oid,&sb);+if(cmit)+describe_commit(&oid,&sb);+elseif(lookup_blob(&oid))+describe_blob(oid,&sb);+else+die(_("%s is neither a commit nor blob"),arg);puts(sb.buf);
@@ -310,6 +310,21 @@ test_expect_success 'describe ignoring a broken submodule' 'grepbrokenout'+test_expect_success'describe a blob at a tag''+echo"make it a unique blob">file&&+gitaddfile&&gitcommit-m"content in file"&&+gittag-a-m"latest annotated tag"unique-file&&+gitdescribeHEAD:file>actual&&+echo"unique-file:file">expect&&+test_cmpexpectactual+'++test_expect_success'describe a blob with commit-ish''+gitcommit--allow-empty-m"empty commit"&&+gitdescribeHEAD:file>actual&&+grepunique-file-1-gactual+'+ test_expect_failureULIMIT_STACK_SIZE'name-rev works in a deep repo''i=1&&whiletest$i-lt8000
From: Jonathan Tan <hidden> Date: 2017-11-14 20:02:15
On Thu, 2 Nov 2017 12:41:48 -0700
Stefan Beller [off-list ref] wrote:
Sometimes users are given a hash of an object and they want to
identify it further (ex.: Use verify-pack to find the largest blobs,
but what are these? or [1])
"This is an interesting endeavor, because describing things is hard."
-- me, upon writing this patch.
When describing commits, we try to anchor them to tags or refs, as these
are conceptually on a higher level than the commit. And if there is no ref
or tag that matches exactly, we're out of luck. So we employ a heuristic
to make up a name for the commit. These names are ambiguous, there might
be different tags or refs to anchor to, and there might be different
path in the DAG to travel to arrive at the commit precisely.
When describing a blob, we want to describe the blob from a higher layer
as well, which is a tuple of (commit, deep/path) as the tree objects
involved are rather uninteresting. The same blob can be referenced by
multiple commits, so how we decide which commit to use? This patch
implements a rather naive approach on this: As there are no back pointers
from blobs to commits in which the blob occurs, we'll start walking from
any tips available, listing the blobs in-order of the commit and once we
found the blob, we'll take the first commit that listed the blob. For
source code this is likely not the first commit that introduced the blob,
but rather the latest commit that contained the blob. For example:
git describe v0.99:Makefile
v0.99-5-gab6625e06a:Makefile
tells us the latest commit that contained the Makefile as it was in tag
v0.99 is commit v0.99-5-gab6625e06a (and at the same path), as the next
commit on top v0.99-6-gb1de9de2b9 ([PATCH] Bootstrap "make dist",
2005-07-11) touches the Makefile.
Let's see how this description turns out, if it is useful in day-to-day
use as I have the intuition that we'd rather want to see the *first*
commit that this blob was introduced to the repository (which can be
achieved easily by giving the `--reverse` flag in the describe_blob rev
walk).
The method of your intuition indeed seems better - could we just have
this from the start?
Alternatively, to me, it seems that listing commits that *introduces*
the blob (that is, where it references the blob, but none of its parents
do) would be the best way. That would then be independent of traversal
order (and we would no longer need to find a tag etc. to tie the blob
to).
If we do that, it seems to me that there is a future optimization that
could get the first commit to the user more quickly - once a commit
without the blob and a descendant commit with the blob is found, that
interval can be bisected, so that the first commit is found in O(log
number of commits) instead of O(commits). But this can be done later.
From: Stefan Beller <hidden> Date: 2017-11-14 20:40:10
On Tue, Nov 14, 2017 at 12:02 PM, Jonathan Tan [off-list ref] wrote:
On Thu, 2 Nov 2017 12:41:48 -0700
Stefan Beller [off-list ref] wrote:
quoted
Sometimes users are given a hash of an object and they want to
identify it further (ex.: Use verify-pack to find the largest blobs,
but what are these? or [1])
"This is an interesting endeavor, because describing things is hard."
-- me, upon writing this patch.
When describing commits, we try to anchor them to tags or refs, as these
are conceptually on a higher level than the commit. And if there is no ref
or tag that matches exactly, we're out of luck. So we employ a heuristic
to make up a name for the commit. These names are ambiguous, there might
be different tags or refs to anchor to, and there might be different
path in the DAG to travel to arrive at the commit precisely.
When describing a blob, we want to describe the blob from a higher layer
as well, which is a tuple of (commit, deep/path) as the tree objects
involved are rather uninteresting. The same blob can be referenced by
multiple commits, so how we decide which commit to use? This patch
implements a rather naive approach on this: As there are no back pointers
from blobs to commits in which the blob occurs, we'll start walking from
any tips available, listing the blobs in-order of the commit and once we
found the blob, we'll take the first commit that listed the blob. For
source code this is likely not the first commit that introduced the blob,
but rather the latest commit that contained the blob. For example:
git describe v0.99:Makefile
v0.99-5-gab6625e06a:Makefile
tells us the latest commit that contained the Makefile as it was in tag
v0.99 is commit v0.99-5-gab6625e06a (and at the same path), as the next
commit on top v0.99-6-gb1de9de2b9 ([PATCH] Bootstrap "make dist",
2005-07-11) touches the Makefile.
Let's see how this description turns out, if it is useful in day-to-day
use as I have the intuition that we'd rather want to see the *first*
commit that this blob was introduced to the repository (which can be
achieved easily by giving the `--reverse` flag in the describe_blob rev
walk).
The method of your intuition indeed seems better - could we just have
this from the start?
Thanks for the review!
This series was written with the mindset, that a user would only ever
want to describe bad blobs. (bad in terms of file size, unwanted content, etc)
With the --reverse you only see the *first* introduction of said blob,
so finding out if it was re-introduced is still not as easy, whereas "when
was this blob last used" which is what the current algorithm does, covers
that case better.
Alternatively, to me, it seems that listing commits that *introduces*
the blob (that is, where it references the blob, but none of its parents
do) would be the best way. That would then be independent of traversal
order (and we would no longer need to find a tag etc. to tie the blob
to).
What if it is introduced multiple times? (either in multiple competing
side branches; or introduced, reverted and re-introduced?)
If we do that, it seems to me that there is a future optimization that
could get the first commit to the user more quickly - once a commit
without the blob and a descendant commit with the blob is found, that
interval can be bisected, so that the first commit is found in O(log
number of commits) instead of O(commits). But this can be done later.
bisection assumes that we only have one "event" going from good to
bad, which doesn't hold true here, as the blob can be there at different
occasions of the history.
From: Jonathan Tan <hidden> Date: 2017-11-14 21:17:38
On Tue, 14 Nov 2017 12:40:03 -0800
Stefan Beller [off-list ref] wrote:
Thanks for the review!
This series was written with the mindset, that a user would only ever
want to describe bad blobs. (bad in terms of file size, unwanted content, etc)
With the --reverse you only see the *first* introduction of said blob,
so finding out if it was re-introduced is still not as easy, whereas "when
was this blob last used" which is what the current algorithm does, covers
that case better.
How does "when was this blob last used" cover reintroduction better? If
you want to check all introductions, then you'll need something like
what I describe (quoted below).
quoted
Alternatively, to me, it seems that listing commits that *introduces*
the blob (that is, where it references the blob, but none of its parents
do) would be the best way. That would then be independent of traversal
order (and we would no longer need to find a tag etc. to tie the blob
to).
What if it is introduced multiple times? (either in multiple competing
side branches; or introduced, reverted and re-introduced?)
Then all of them should be listed.
quoted
If we do that, it seems to me that there is a future optimization that
could get the first commit to the user more quickly - once a commit
without the blob and a descendant commit with the blob is found, that
interval can be bisected, so that the first commit is found in O(log
number of commits) instead of O(commits). But this can be done later.
bisection assumes that we only have one "event" going from good to
bad, which doesn't hold true here, as the blob can be there at different
occasions of the history.
Yes, bisection would only be useful to output one commit more quickly.
We will still need to exhaustively search for all commits if the user
needs more than one.
From: Stefan Beller <hidden> Date: 2017-11-02 19:42:20
The function `describe` has already a variable named `oid` declared at
the beginning of the function for an object id. Do not shadow that
variable with a pointer to an object id.
Signed-off-by: Stefan Beller <redacted>
---
builtin/describe.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -392,11 +392,11 @@ static void describe(const char *arg, int last_one)if(unannotated_cnt)die(_("No annotated tags can describe '%s'.\n""However, there were unannotated tags: try --tags."),-oid_to_hex(oid));+oid_to_hex(cmit_oid));elsedie(_("No tags can describe '%s'.\n""Try --always, or create some tags."),-oid_to_hex(oid));+oid_to_hex(cmit_oid));}QSORT(all_matches,match_cnt,compare_pt);
From: Jacob Keller <hidden> Date: 2017-11-03 00:24:21
On Thu, Nov 2, 2017 at 12:41 PM, Stefan Beller [off-list ref] wrote:
Thanks for the discussion on v2[1].
Interdiff is below, just fixing minor things.
We'll keep the original algorithm for now, deferring an improvement on
the algorithm front towards a future developer.
I agree, "something" is better than "nothing", and we can work to
improve "something" in the future, especially after we get more real
use and see what people think. Only question would be how much do we
need to document the current behavior might be open for improvement?
quoted hunk
Thanks,
Stefan
[1] https://public-inbox.org/git/20171031211852.13001-1-sbeller@google.com/
Stefan Beller (7):
t6120: fix typo in test name
list-objects.c: factor out traverse_trees_and_blobs
revision.h: introduce blob/tree walking in order of the commits
builtin/describe.c: rename `oid` to avoid variable shadowing
builtin/describe.c: print debug statements earlier
builtin/describe.c: factor out describe_commit
builtin/describe.c: describe a blob
Documentation/git-describe.txt | 13 +++-
Documentation/rev-list-options.txt | 5 ++
builtin/describe.c | 125 ++++++++++++++++++++++++++++---------
list-objects.c | 52 +++++++++------
revision.c | 2 +
revision.h | 3 +-
t/t6100-rev-list-in-order.sh | 47 ++++++++++++++
t/t6120-describe.sh | 17 ++++-
8 files changed, 214 insertions(+), 50 deletions(-)
create mode 100755 t/t6100-rev-list-in-order.sh
@@ -463,7 +465,7 @@ static void describe_blob(struct object_id oid, struct strbuf *dst){structrev_inforevs;structargv_arrayargs=ARGV_ARRAY_INIT;-structprocess_commit_datapcd={null_oid,oid,dst};+structprocess_commit_datapcd={null_oid,oid,dst,&revs};argv_array_pushl(&args,"internal: The first arg is not parsed","--all","--reflog",/* as many starting points as possible */
@@ -497,14 +499,12 @@ static void describe(const char *arg, int last_one)die(_("Not a valid object name %s"),arg);cmit=lookup_commit_reference_gently(&oid,1);-if(cmit){+if(cmit)describe_commit(&oid,&sb);-}else{-if(lookup_blob(&oid))-describe_blob(oid,&sb);-else-die(_("%s is neither a commit nor blob"),arg);-}+elseif(lookup_blob(&oid))+describe_blob(oid,&sb);+else+die(_("%s is neither a commit nor blob"),arg);puts(sb.buf);
@@ -319,7 +319,7 @@ test_expect_success 'describe a blob at a tag' 'test_cmpexpectactual'-test_expect_success'describe a surviving blob''+test_expect_success'describe a blob with commit-ish''gitcommit--allow-empty-m"empty commit"&&gitdescribeHEAD:file>actual&&grepunique-file-1-gactual