Hi Emily,
On Wed, 19 Feb 2020, Emily Shaffer wrote:
> In some cases, it could be that the user is having a problem with an
> object which isn't present in their normal object directory. We can get
> a hint that that might be the case by examining the list of alternates
> where their object may be stored instead. Since paths to alternates may
> be sensitive, we'll instead count how many alternates have been
> specified and note how many of them exist or are broken.
>
> While object-cache.h describes a function "foreach_alt_odb()", this
> function does not provide information on broken alternates, which are
> skipped over in "link_alt_odb_entry()". Since the goal is to identify
> missing alternates, we can gather the contents of
> .git/objects/info/alternates manually.
Makes sense.
> diff --git a/bugreport.c b/bugreport.c
> index 1d61e0f642..1640a71086 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -255,6 +255,48 @@ static void get_object_info_summary(struct strbuf *obj_info, int nongit)
> strbuf_release(&dirpath);
> }
>
> +static void get_alternates_summary(struct strbuf *alternates_info, int nongit)
> +{
> + struct strbuf alternates_path = STRBUF_INIT;
> + struct strbuf alternate = STRBUF_INIT;
I am not sure that those variables and the parameter need to repeat
"alternate", I find it rather distracting. If it were me, I would rename
the parameter to `info`, the first strbuf to `path` and the second to
`line`. This function is so specific to read the alternates file that it
is quite obvious what their roles are.
> + FILE *file;
> + size_t exists = 0, broken = 0;
> +
> + if (nongit) {
> + strbuf_addstr(alternates_info,
> + "not run from a git repository - alternates unavailable\n");
> + return;
> + }
> +
> + strbuf_addstr(&alternates_path, get_object_directory());
> + strbuf_complete(&alternates_path, '/');
> + strbuf_addstr(&alternates_path, "info/alternates");
> +
> + file = fopen(alternates_path.buf, "r");
> + if (!file) {
> + strbuf_addstr(alternates_info, "No alternates file found.\n");
> + strbuf_release(&alternates_path);
> + return;
> + }
> +
> + while (strbuf_getline(&alternate, file) != EOF) {
> + if (!access(alternate.buf, F_OK))
Should we make sure that the alternate is actually valid objects directory
here? Like, look whether it is a directory, not a file or a (possibly
dangling) symbolic link. This seems to be the only check
`alt_odb_usable()` performs, so that should probably be good enough here,
too.
> + exists++;
> + else
> + broken++;
> + }
> +
> + strbuf_addf(alternates_info,
> + "%zd alternates found (%zd working, %zd broken)\n",
Sadly, `%zd` is not portable. Therefore, `pu` (and `es/bugreport`) do not
even _build_ on Windows. I need this to make it work:
-- snip --
diff --git a/bugreport.c b/bugreport.c
index 3770aa73fae..5033668e22f 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -287,10 +287,11 @@ static void get_alternates_summary(struct strbuf *alternates_info, int nongit)
}
strbuf_addf(alternates_info,
- "%zd alternates found (%zd working, %zd broken)\n",
- exists + broken,
- exists,
- broken);
+ "%"PRIuMAX" alternates found "
+ "(%"PRIuMAX" working, %"PRIuMAX" broken)\n",
+ (uintmax_t)(exists + broken),
+ (uintmax_t)exists,
+ (uintmax_t)broken);
fclose(file);
strbuf_release(&alternate);
-- snap --
Could you incorporate that into the next iteration, please?
Thanks,
Dscho
> + exists + broken,
> + exists,
> + broken);
> +
> + fclose(file);
> + strbuf_release(&alternate);
> + strbuf_release(&alternates_path);
> +}
> +
> static const char * const bugreport_usage[] = {
> N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
> NULL
> @@ -355,6 +397,9 @@ int cmd_main(int argc, const char **argv)
> get_header(&buffer, "Object Info Summary");
> get_object_info_summary(&buffer, nongit_ok);
>
> + get_header(&buffer, "Alternates");
> + get_alternates_summary(&buffer, nongit_ok);
> +
> report = fopen_for_writing(report_path.buf);
>
> if (report == NULL) {
> --
> 2.25.0.265.gbab2e86ba0-goog
>
>
